Re: [PATCH 4/4] linux: Add pidfd_getpid

[Luca Boccassi <luca.boccassi@gmail.com>]

On Wed, 19 Apr 2023 at 15:11, Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> On 19/04/23 11:01, Luca Boccassi wrote:
> > On Wed, 19 Apr 2023 at 14:56, Adhemerval Zanella Netto
> > <adhemerval.zanella@linaro.org> wrote:
> >> On 19/04/23 07:29, Luca Boccassi wrote:
> >>>> +parse_fdinfo (const char *l, void *arg)
> >>>> +{
> >>>> +  enum { fieldlen = sizeof ("Pid:") - 1 };
> >>>> +  if (strncmp (l, "Pid:", fieldlen) != 0)
> >>>> +    return true;
> >>>> +
> >>>> +  l += fieldlen;
> >>>> +
> >>>> +  char *endp;
> >>>> +  unsigned long int n = strtoul (l, &endp, 10);
> >>>> +  if (l == endp || n > INT_MAX)
> >>>> +    return true;
> >>>> +
> >>>> +  *(pid_t *)arg = n;
> >>>> +  return false;
> >>>> +}
> >>>> +
> >>>> +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';
> >>>> +
> >>>> +  pid_t pid;
> >>>> +  if (procutils_read_file (fdinfoname, parse_fdinfo, &pid) == -1)
> >>>> +    return -1;
> >>>> +
> >>>> +  return pid;
> >>>
> >>> Having implemented this parsing by hand across 3 projects, it is great
> >>> to see a glibc helper coming.
> >>>
> >>> However, please handle the case of Pid being 0 and -1 explicitly, and
> >>> return a recognizable errno. fdinfo listing 0 means the pidfd cannot be
> >>> resolved because it's in a separate pid namespace, so something EREMOTE
> >>> would suffice. -1 means the process exited, so ESRCH seems like the
> >>> right error. The distiction between these cases and other errors is
> >>> important to userspace where we do process tracking, like
> >>> systemd/dbus/polkit.
> >>
> >> Ok, it should a matter to handle the 'Pid:' as signed. However, I am not
> >> sure if it is really worth to set the errno in cases, since the information
> >> will already available from the function return itself.
> >
> > The function returns the pid, so the same information wouldn't be
> > available. Eg. returning -1 because you can't read proc because of
> > selinux permissions is different from returning -1 because the process
> > was reaped and fdinfo contained -1 so it was just passed through. We
> > need to know this distinction in userspace to handle it properly.
> > Setting the errno accordingly seems the best way to do that to me.
>
> I am kind worried where such errors might clash with other open/read/close
> used in parsing, but at least it seems ESRCH should not be generated in such
> cases.  I am still not sure about EREMOTE, maybe we should just map the
> pid equal 0 to -1/EREMOTE.

Yes, -1/EREMOTE makes sense to me.

Kind regards,
Luca Boccassi