Re: [PATCH v2 3/3] linux: Add pidfd_getpid

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

> This interface allows to obtain the associated pid ID from the
> process file descriptor.  It is done by parsing the procps fdinfo
> information.  Its prototype is:
>
>    pid_t pidfd_getpid (int fd)
>
> It returns the associated pid or -1 in case of an error and set the
> errno accordingly.  The possible errno values are the smae from
> open, read, and close (used on procps parsing), along with:
>
>    - EINVAL if the FP is negative (similar to fexecve).
>
>    - EBADF if the FD does not have a PID associated of if the fdinfo
>      fields contains a value larger than pid_t.
>
>    - EREMOTE if the PID is in a separate namespace.
>
>    - ESRCH if the process is already terminated.

Could you add a manual entry for this?

The documentation for EREMOTE (also in the comment in the code) should
probably say that this is returned if there is no PID to denote the
process in the current namespace.  I assume that the PID is available
from an outer namespace (even across PID namespace boundaries), but not
necessarily in the other direction.

> diff --git a/sysdeps/unix/sysv/linux/pidfd_getpid.c b/sysdeps/unix/sysv/linux/pidfd_getpid.c
> new file mode 100644
> index 0000000000..d0c7987791
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/pidfd_getpid.c
> @@ -0,0 +1,94 @@

> +static bool
> +parse_fdinfo (const char *l, void *arg)
> +{
> +  enum { fieldlen = sizeof ("Pid:") - 1 };
> +  if (strncmp (l, "Pid:", fieldlen) != 0)
> +    return true;
> +
> +  l += fieldlen;
> +
> +  char *endp;
> +  long int n = strtol (l, &endp, 10);
> +  if (l == endp || n > INT_MAX)
> +    return true;

Should this use strtoul?  Otherwise I'm not sure the overflow check will
work on 32-bit.

Rest of the implementation looks okay, but the kernel should really
provide an ioctl or similar for this.

> diff --git a/sysdeps/unix/sysv/linux/procutils.c b/sysdeps/unix/sysv/linux/procutils.c
> new file mode 100644
> index 0000000000..4909afeae1
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/procutils.c
> @@ -0,0 +1,99 @@
> +/* Utilities functions to read/parse Linux procfs and sysfs.

> +int
> +procutils_read_file (const char *filename, procutils_closure_t closure,
> +		     void *arg)
> +{
> +  enum { buffer_size = 1024 };
> +  char buffer[buffer_size];
> +  char *buffer_end = buffer + buffer_size;
> +  char *cp = buffer_end;
> +  char *re = buffer_end;
> +
> +  int fd = __open64_nocancel (filename, O_RDONLY | O_CLOEXEC);
> +  if (fd == -1)
> +    return -1;
> +
> +  char *l;
> +  while ((l = next_line (fd, buffer, &cp, &re, buffer_end)) != NULL)
> +    if (!closure (l, arg))
> +      break;
> +
> +  __close_nocancel_nostatus (fd);
> +
> +  return 0;
> +}

Incomplete error reporting?  Any read failure doesn't make it to the
caller.

The callback interface will be easier to use if you have it return int
and return the callback return value if it is non-zero, I think.

> diff --git a/sysdeps/unix/sysv/linux/tst-pidfd.c b/sysdeps/unix/sysv/linux/tst-pidfd.c
> index 64d8a2ef40..2a73328ef1 100644
> --- a/sysdeps/unix/sysv/linux/tst-pidfd.c
> +++ b/sysdeps/unix/sysv/linux/tst-pidfd.c
> @@ -18,6 +18,7 @@

It would be nice to have a check for the EREMOTE case.

Thanks,
Florian