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

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

On 02/05/23 13:07, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> This interface allows to obtain the associated pid ID from the
>> 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
>> 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
>>      fields contains a value larger than pid_t.
>>
>>    - EREMOTE if the PID is in a separate namespace.
>>
>>    - ESRCH if the process is already terminated.
> 
> Could you add a manual entry for this?
> 
> The documentation for EREMOTE (also in the comment in the code) should
> probably say that this is returned if there is no PID to denote the
> process in the current namespace.  I assume that the PID is available
> from an outer namespace (even across PID namespace boundaries), but not
> necessarily in the other direction.

Ok, I will update the patch with a manual entry. I will also add a testcase
for EREMOTE.

> 
>> diff --git a/sysdeps/unix/sysv/linux/pidfd_getpid.c b/sysdeps/unix/sysv/linux/pidfd_getpid.c
>> new file mode 100644
>> index 0000000000..d0c7987791
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/pidfd_getpid.c
>> @@ -0,0 +1,94 @@
> 
>> +static bool
>> +parse_fdinfo (const char *l, void *arg)
>> +{
>> +  enum { fieldlen = sizeof ("Pid:") - 1 };
>> +  if (strncmp (l, "Pid:", fieldlen) != 0)
>> +    return true;
>> +
>> +  l += fieldlen;
>> +
>> +  char *endp;
>> +  long int n = strtol (l, &endp, 10);
>> +  if (l == endp || n > INT_MAX)
>> +    return true;
> 
> Should this use strtoul?  Otherwise I'm not sure the overflow check will
> work on 32-bit.

Ack.

> 
> Rest of the implementation looks okay, but the kernel should really
> provide an ioctl or similar for this.

It would be the best option, although assuming procps support it is just
a mater to correct parsing it (although such things is frequently subject
to subtle bugs).

> 
>> diff --git a/sysdeps/unix/sysv/linux/procutils.c b/sysdeps/unix/sysv/linux/procutils.c
>> new file mode 100644
>> index 0000000000..4909afeae1
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/procutils.c
>> @@ -0,0 +1,99 @@
>> +/* Utilities functions to read/parse Linux procfs and sysfs.
> 
>> +int
>> +procutils_read_file (const char *filename, procutils_closure_t closure,
>> +		     void *arg)
>> +{
>> +  enum { buffer_size = 1024 };
>> +  char buffer[buffer_size];
>> +  char *buffer_end = buffer + buffer_size;
>> +  char *cp = buffer_end;
>> +  char *re = buffer_end;
>> +
>> +  int fd = __open64_nocancel (filename, O_RDONLY | O_CLOEXEC);
>> +  if (fd == -1)
>> +    return -1;
>> +
>> +  char *l;
>> +  while ((l = next_line (fd, buffer, &cp, &re, buffer_end)) != NULL)
>> +    if (!closure (l, arg))
>> +      break;
>> +
>> +  __close_nocancel_nostatus (fd);
>> +
>> +  return 0;
>> +}
> 
> Incomplete error reporting?  Any read failure doesn't make it to the
> caller.

Ack, I will fix to report read errors.

> 
> The callback interface will be easier to use if you have it return int
> and return the callback return value if it is non-zero, I think.

Ack.

> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-pidfd.c b/sysdeps/unix/sysv/linux/tst-pidfd.c
>> index 64d8a2ef40..2a73328ef1 100644
>> --- a/sysdeps/unix/sysv/linux/tst-pidfd.c
>> +++ b/sysdeps/unix/sysv/linux/tst-pidfd.c
>> @@ -18,6 +18,7 @@
> 
> It would be nice to have a check for the EREMOTE case.
> 
> Thanks,
> Florian
>