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

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> This interface allows to obtain the associated pid ID from the

“process ID”

> 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

Typos: “and set[s] [] errno accordingly.”

“are [those] 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

“associated o[r] if”

>      fields contains a value larger than pid_t.
>
>    - EREMOTE if the PID is in a separate namespace.
>
>    - ESRCH if the process is already terminated.

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

> +pid_t
> +pidfd_getpid (int fd)
> +{
> +  if (__glibc_unlikely (fd < 0))
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }
> +
> +  char fdinfoname[FDINFO_FILENAME_LEN];
> +
> +  char *p = mempcpy (fdinfoname, FDINFO_TO_FILENAME_PREFIX,
> +		     strlen (FDINFO_TO_FILENAME_PREFIX));
> +  *_fitoa_word (fd, p, 10, 0) = '\0';
> +
> +  struct parse_fdinfo_t fdinfo = { .found = false, .pid = -1 };
> +  if (procutils_read_file (fdinfoname, parse_fdinfo, &fdinfo) == -1)
> +    /* The fdinfo contains an invalid 'Pid:' value.  */
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EBADF);
> +
> +  /* The FD does not have a 'Pid:' entry associated.  */
> +  if (!fdinfo.found)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EBADF);
> +
> +  /* The pidfd cannot be resolved because it is in a separate pid
> +     namespace.  */
> +  if (fdinfo.pid == 0)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (EREMOTE);
> +
> +  /* A negative value means the process is terminated.  */
> +  if (fdinfo.pid < 0)
> +    return INLINE_SYSCALL_ERROR_RETURN_VALUE (ESRCH);
> +
> +  return fdinfo.pid;

We are making a ot of error codes here.  How likely is it that they
match what a future ioctl will do?

> diff --git a/sysdeps/unix/sysv/linux/tst-pidfd.c b/sysdeps/unix/sysv/linux/tst-pidfd.c
> index 64d8a2ef40..a03de4b290 100644
> --- a/sysdeps/unix/sysv/linux/tst-pidfd.c
> +++ b/sysdeps/unix/sysv/linux/tst-pidfd.c

> +  /* Check if pidfd_getpid returns ESRCH for exited subprocess.  */
> +  {
> +    int pidfd = pidfd_fork (0);
> +    if (pidfd == 0)
> +      exit (EXIT_SUCCESS);

Should be _exit, so that the atexit handlers do not run.

I think the main remaining question is errno alignment with a future
direct kernel interface for this.

Thanks,
Florian