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

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

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

> +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.

> +   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"?

> +  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.

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

-- 
Kind regards,
Luca Boccassi

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]