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

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

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 CLOSURE should return false if the read should continue, or false
>> +   if the function should stop.
> 
> Did you mean "true if the read should continue"?

Indeed.

> 
>> +  TEST_COMPARE (pidfd_getpid (INT_MAX), -1);
>> +  {
>> +    pid_t querypid = pidfd_getpid (pidfd);
>> +    TEST_COMPARE (querypid, pid);
>> +  }
> 
> I think it would be a good idea to add a negative test, eg for a
> process that exited or so.

Alright, I will add a more comprehensible test.

> 
> (I am not subscribed to the list, so please CC directly if needed)
>