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

Andrew Burgess
2025-05-23T18:15:23Z

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