Re: pidfd_open.2: PIDFD_NONBLOCK is not defined by the listed headers

[Alejandro Colomar <alx@kernel.org>]

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

Hi Emanuele,

Do you have any updates of this patch?  Thanks!

Have a lovely day!
Alex

On Mon, May 20, 2024 at 08:35:48AM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 20/05/24 05:53, Alejandro Colomar wrote:
> > Oops, I mistyped the glibc list.  Below is included the original email.
> > 
> > ---
> > 
> > On Mon, May 20, 2024 at 07:02:39AM GMT, Emanuele Torre wrote:
> >> Hello.
> > 
> > Hi Emanuele,
> > 
> >> pidfd_open(2) only lists sys/syscall.h and unistd.h in its SYNOPSYS:
> >>
> >>   SYNOPSIS
> >>          #include <sys/syscall.h>      /* Definition of SYS_* constants */
> >>          #include <unistd.h>
> >>
> >>          int syscall(SYS_pidfd_open, pid_t pid, unsigned int flags);
> >>
> >>          Note:  glibc provides no wrapper for pidfd_open(), necessitating
> >>          the use of syscall(2).
> >>
> >> Then it mentions PIDFD_NONBLOCK as one of its flags:
> >>
> >>   PIDFD_NONBLOCK (since Linux 5.10)
> >>          Return  a nonblocking file descriptor.  If the process referred
> >>          to by the file descriptor has not yet terminated, then  an  at‐
> >>          tempt to wait on the file descriptor using waitid(2) will imme‐
> >>          diately return the error EAGAIN rather than blocking.
> >>
> >> But PIDFD_NONBLOCK is not defined in any of the listed headers.
> > 
> > Hmmm.  Thanks!  We need to add its header.
> > 
> >> I have noticed that PIDFD_NONBLOCK is the same value as O_NONBLOCK,
> >> so perhaps this flag could be listed as
> >>
> >>   O_NONBLOCK or PIDFD_NONBLOCK (since Linux 5.10)
> >>
> >> like O_NDELAY and O_NONBLOCK in open.2.
> >>
> >> This way the user would know that O_NONBLOCK may be used instead of
> >> PIDFD_NONBLOCK if PIDFD_NONBLOCK is not available.
> > 
> > No.  That's an implementation detail, which shouldn't be abused.
> > 
> >> I have also noticed that GNU libc (in its linux-api-headers submodule)
> >> provides a linux/pidfd.h header that just defines PIDFD_NONBLOCK as
> >> O_NONBLOCK, perhaps another solution would be to list in linux/pidfd.h
> >> in the synopsis and say it is required to use PIDFD_NONBLOCK.
> > 
> > Yep, that's the kernel uapi header.  I didn't know glibc redistributes
> > those.
> > 
> > Anyway, we should indeed include <linux/pidfd.h> for this macro.
> 
> The glibc provides the pidfd_open, pidfd_getfd, and pidfd_send_signal
> since 2.36 [1][2][3], and pidfd_getpid since 2.39 [4].  It also provides the
> pidfd_spawn and pidfd_spawp [5], which are similar to posix_spawn, but
> return return a pidfd.
> 
> > 
> >> Then, I also noticed that GNU libc now also provides the sys/pidfd.h
> >> header with the definition of PIDFD_NONBLOCK, and prototypes for
> >> pidfd_open, pidfd_send_signal, pidfd_getfd, and also a prototype for
> >> pidfd_getpid that is an helper function that parses the "Pid:" field
> >> from /proc/self/fdinfo/FD and currently does not have a man page.
> > 
> > Hmmm, I've CCed glibc for a question:  When you provide a macro like
> > this one, without providing a syscall wrapper, should we include the
> > glibc header or the kernel one?  Do you provide all kernel uapi macros,
> > or just select ones?
> 
> For pidfd function we decided to add the function on sys/pidfd.h which
> is a distinct header.  Maybe we should follow other kernel header 
> integration and include it if existent and only define the required
> macros if not existent (like we do on mount.h). 
> 
> > 
> > As far as I understand (I have never tried to use it in a program),
> > 
> >   pid_t pid = pidfd_getfd(pidfd);
> > 
> > Is equivalent to the following command in shell:
> > 
> >   pid=$(grep -Pom1 '^Pid:\t\K.*' proc/self/fdinfo"$pidfd" || echo -1)
> 
> Yes, and it sets errno depending parsing and 'Pid:' value (you can check
> on the pidfd_getfd documentation on glibc manual).
> 
> 
> [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=97f5d19c45799e3abedef771430b5562f1b8764f
> [2] https://sourceware.org/git/?p=glibc.git;a=commit;h=32dd8c251a431c90451092653f0231a4ad2665e5
> [3] https://sourceware.org/git/?p=glibc.git;a=commit;h=56cf9e8eec3bdc0ce44efeda373de9d6b825ea1e
> [4] https://sourceware.org/git/?p=glibc.git;a=commit;h=e7190fc73dbc8a1f8f94f8ccacd9a190fa5e609c
> [5] https://sourceware.org/git/?p=glibc.git;a=commit;h=0d6f9f626521678f330f8bfee89e1cdb7e2b1062
> 
> > 
> >>
> >> So probably the best solution is to just make the pidfd_open(2),
> >> pidfd_send_signal(2), and pidfd_getfd(2) man pages tell users to include
> >> sys/pidfd.h and call the GNU libc functions instead of including
> >> sys/syscall.h and unistd.h and calling syscall(2) directly; now that
> >> sys/pidfd.h exists.
> > 
> > Ahh, interesting.  I'm using glibc 2.38 and still don't have that one.
> > It seems added in 2.39.  We can directly document that in
> > pidfd_getfd(2).
> > 
> >> And maybe to also add a pidfd_getpid(3) man page for the new pidfd
> >> helper function.
> > 
> > No, usually we document the glibc wrapper in man2, unless there's a big
> > difference between the kernel syscall and the glibc wrapper.
> > 
> > Thanks for the detailed report!
> > 
> > Have a lovely day!
> > Alex
> > 
> >>
> >>
> >> o/
> >>  emanuele6
> > 

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]