Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat

[Andrew Burgess <aburgess@redhat.com>]

Fabian Kilger <kilger@sec.in.tum.de> writes:

> Thanks for reviewing, I'll be including these in v2. I have questions
> regarding some style recommendations, where I feel like I'd be breaking
> the style of the remaining file.
>
> On 5/23/25 20:14, Andrew Burgess wrote:
>> Fabian Kilger <kilger@sec.in.tum.de> writes:
>> 
>> Thanks for working on this.  This mostly looks good, I have just a few
>> style issues that I think should be fixed, but it's all pretty minor.
>> 
>> 
>>> The new algorithm to look for a build-id-based debug file
>>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba)
>>> makes use of fileio_stat. As fileio_stat was not supported by
>>> linux-namespace.c, all stat calls would be performed on the host
>>> and not inside the namespace
>>>
>>> ---
>>>  gdb/linux-nat.c            | 14 ++++++++
>>>  gdb/linux-nat.h            |  3 ++
>>>  gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++
>>>  gdb/nat/linux-namespaces.h |  6 ++++
>>>  4 files changed, 94 insertions(+)
>>>
>>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>>> index 3f252370c7b..478a7977c4d 100644
>>> --- a/gdb/linux-nat.c
>>> +++ b/gdb/linux-nat.c
>>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename,
>>>    return fd;
>>>  }
>>>  
>>> +/* Implementation of to_fileio_stat.  */
>>> +int
>> 
>> Add an empty line after the comment please to match the other functions
>> in this file.
>> 
>>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename,
>>> +			       struct stat *sb, fileio_error *target_errno)
>>> +{
>>> +  int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf),
>>> +			    filename, sb);
>> 
>> There's no need to wrap this argument list.
>> 
>
> I've tried copying the style of the other functions in the file. All
> fileio_X functions wrap the argument list after linux_nat_fileio_pid_of.
> Specifically also fileio_unlink, which has even less arguments. As such,
> I feel like not wrapping would break with the style of the other
> functions. Should I still not wrap it?

My understanding is that the "correct" style takes precedent over
matching "incorrect" style in a file.  The hope is that, over time, as
people touch the code, instances of incorrect style will slowly be
fixed.  But if we keep adding more incorrect style to match existing
code, then things will never get better.

Worse, due to copy&paste, the more incorrectly styled code we have, the
more the incorrect styling spreads from file to file.

>
>>> +
>>> +  if (r == -1)
>>> +    *target_errno = host_to_fileio_error (errno);
>>> +
>>> +  return r;
>>> +}
>>> +
>>>  /* Implementation of to_fileio_readlink.  */
>>>  
>>>  std::optional<std::string>
>>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
>>> index b630b858e34..42d1ec142b3 100644
>>> --- a/gdb/linux-nat.h
>>> +++ b/gdb/linux-nat.h
>>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target
>>>  		     const char *filename,
>>>  		     fileio_error *target_errno) override;
>>>  
>>> +  int fileio_stat (struct inferior *inf, const char *filename,
>>> +		   struct stat *sb, fileio_error *target_errno) override;
>>> +
>>>    int fileio_unlink (struct inferior *inf,
>>>  		     const char *filename,
>>>  		     fileio_error *target_errno) override;
>>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c
>>> index 19a05eec905..aa74e9df950 100644
>>> --- a/gdb/nat/linux-namespaces.c
>>> +++ b/gdb/nat/linux-namespaces.c
>>> @@ -233,6 +233,12 @@ enum mnsh_msg_type
>>>         MNSH_RET_INT.  */
>>>      MNSH_REQ_SETNS,
>>>  
>>> +    /* A request that the helper call stat.  The single
>>> +       argument (the filename) should be passed in BUF, and
>>> +       should include a terminating NUL character.  The helper
>>> +       should respond with a MNSH_RET_INTSTR.  */
>>> +    MNSH_REQ_STAT,
>>> +
>>>      /* A request that the helper call open.  Arguments should
>>>         be passed in BUF, INT1 and INT2.  The filename (in BUF)
>>>         should include a terminating NUL character.  The helper
>>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type,
>>>        debug_printf ("ERROR");
>>>        break;
>>>  
>>> +    case MNSH_REQ_STAT:
>>> +      debug_printf ("STAT");
>>> +      break;
>>> +
>>>      case MNSH_REQ_SETNS:
>>>        debug_printf ("SETNS");
>>>        break;
>>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype)
>>>    return mnsh_return_int (sock, result, errno);
>>>  }
>>>  
>>> +
>>> +/* Handle a MNSH_REQ_STAT message.  Must be async-signal-safe.  */
>>> +
>>> +static ssize_t
>>> +mnsh_handle_stat(int sock, const char *filename)
>>> +{
>>> +  struct stat sb;
>>> +  int stat_ok = stat(filename, &sb);
>> 
>> Space needed after 'stat'.
>> 
>>> +
>>> +  return mnsh_return_intstr(sock, stat_ok, &sb,
>>> +			    stat_ok == -1 ? 0 : sizeof (sb),
>>> +			    errno);
>> 
>> Space needed after 'mnsh_return_intstr'.
>> 
>>> +}
>>> +
>>>  /* Handle a MNSH_REQ_OPEN message.  Must be async-signal-safe.  */
>>>  
>>>  static ssize_t
>>> @@ -574,6 +598,11 @@ mnsh_main (int sock)
>>>  		response = mnsh_handle_setns (sock, fd, int1);
>>>  	      break;
>>>  
>>> +	    case MNSH_REQ_STAT:
>>> +	      if (size > 0 && buf[size - 1] == '\0')
>>> +		response = mnsh_handle_stat(sock, buf);
>> 
>> Space needed after 'mnsh_handle_stat'
>> 
>>> +	      break;
>>> +
>>>  	    case MNSH_REQ_OPEN:
>>>  	      if (size > 0 && buf[size - 1] == '\0')
>>>  		response = mnsh_handle_open (sock, buf, int1, int2);
>>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void)
>>>    mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \
>>>  		     filename, strlen (filename) + 1)
>>>  
>>> +#define mnsh_send_stat(helper, filename) \
>>> +  mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \
>>> +		     filename, strlen (filename) + 1)
>>> +
>>>  #define mnsh_send_unlink(helper, filename) \
>>>    mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \
>>>  		     filename, strlen (filename) + 1)
>>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid)
>>>    return MNSH_FS_HELPER;
>>>  }
>>>  
>>> +
>>> +/* See nat/linux-namespaces.h.  */
>>> +int
>> 
>> Move the empty line from before the comment to after the comment please.
>> 
>>> +linux_mntns_stat (pid_t pid, const char *filename,
>>> +		  struct stat *sb)
>>> +{
>>> +  enum mnsh_fs_code access = linux_mntns_access_fs (pid);
>>> +  struct linux_mnsh *helper;
>>> +  int stat_ok, error;
>>> +  ssize_t size;
>> 
>> The declarations should be moved inline below.
>
> Here I've also copied the style of the remaining functions: All
> functions in that file declare the variables at the beginning in the
> exact same fashion. See linux_mntns_open_cloexec right below it, for
> example.

As above.  Much of this code originated from when GDB was a C project,
at which time the GDB style was that everything should be declared at
the start of the scope, even when C itself no longer required that.

Shortly after the switch to C++ the style changed so that things should
be declared as close to the first use as possible.


You're right to check these things.  And it can be weird to go against
the file's existing style, but hopefully, over time, GDB will become
more consistent.

Thanks,
Andrew