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 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.

> +
> +  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.

> +
> +  if (access == MNSH_FS_ERROR)
> +    return -1;
> +
> +  if (access == MNSH_FS_DIRECT)
> +    return stat(filename, sb);

Space needed after 'stat'.

> +
> +  gdb_assert (access == MNSH_FS_HELPER);
> +
> +  helper = linux_mntns_get_helper ();
> +
> +  size = mnsh_send_stat (helper, filename);
> +  if (size < 0)
> +    return -1;
> +
> +  size = mnsh_recv_intstr (helper, &stat_ok, &error, sb, sizeof (*sb));
> +
> +  if (size < 0)
> +    {
> +      stat_ok = -1;
> +      errno = error;
> +    }
> +  else
> +    gdb_assert (stat_ok == -1 || size == sizeof (*sb));
> +
> +  return stat_ok;
> +}
> +
>  /* See nat/linux-namespaces.h.  */
>  
>  int
> diff --git a/gdb/nat/linux-namespaces.h b/gdb/nat/linux-namespaces.h
> index 4327292950b..825cb27eb2d 100644
> --- a/gdb/nat/linux-namespaces.h
> +++ b/gdb/nat/linux-namespaces.h
> @@ -58,6 +58,12 @@ enum linux_ns_type
>  
>  extern int linux_ns_same (pid_t pid, enum linux_ns_type type);
>  
> +/* Like stat(2), but in the mount namespace of process
> +   PID.  */

This comment doesn't need to wrap.

Thanks,
Andrew



> +
> +extern int linux_mntns_stat (pid_t pid, const char *filename,
> +			     struct stat *sb);
> +
>  /* Like gdb_open_cloexec, but in the mount namespace of process
>     PID.  */
>  
> -- 
> 2.49.0