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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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.