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:
signature.asc
Description: PGP signature
