Date: Thu, 03 Sep 2020 12:25:19 -0400 From: "Stefan O'Rear" <sorear@...tmail.com> To: musl@...ts.openwall.com Subject: Re: [PATCH 07/14] Emulate wait4 using waitid On Thu, Sep 3, 2020, at 11:49 AM, Rich Felker wrote: > On Thu, Sep 03, 2020 at 07:23:02AM -0400, Stefan O'Rear wrote: > > riscv32 and future architectures lack wait4. > > --- > > src/internal/wait4_waitid.h | 1 + > > src/linux/__wait4_waitid.c | 52 +++++++++++++++++++++++++++++++++++++ > > src/linux/wait4.c | 5 ++++ > > src/process/waitpid.c | 6 +++++ > > src/stdio/pclose.c | 6 +++++ > > src/unistd/faccessat.c | 6 ++++- > > 6 files changed, 75 insertions(+), 1 deletion(-) > > create mode 100644 src/internal/wait4_waitid.h > > create mode 100644 src/linux/__wait4_waitid.c > > I really don't like introducing a new src/internal file for this. If I agree. > the code needs to be shared it should probably just be in wait4.c, > renamed to __wait4, declared in src/include/sys/wait.h according to > the usual pattern for exposing namespace-safe versions of > namespace-violating functions. Currently waitpid and waitid are cancellation points (required by POSIX) but wait4 is not, which is why I did not have anything else call a function with exactly wait4 semantics. > Ideally I'd like to just not need it, but I'm not sure that's > possible: > > - faccessat throws away the status and doesn't have any need for the > status or idtype emulation code. It could easily be just different > syscalls in #ifdef/#else paths. I think that is exactly what my patch does? > - pclose does need status but not the idtype part. but it's only > listening for process termination not all the other weird stuff. I'm > not sure if it would make sense to construct status inline here. It > might if we had a macro like the glibc "inverse-W" macro (I forget > its name) to construct a wait status from parts; if so this could > later be considered for a public API (previously requested). Earlier I had the wait status conversion code broken out separately but it seemed not worth it for just pclose. > - waitpid and wait4 at least need the idtype and status construction. > > Unfortunately, wait4 also needs the rusage conversion, which waitpid > doesn't want. This kinda defeats my idea of just exposing __wait4 from > wait4.c. > > A side observation to keep in mind is that passing the argument cp > doesn't really help optimize anything; it only worked well in > c2feda4e2e because the function there is inline. Of the 4 callers you > have, faccessat and pclose have hard requirements not to be a > cancellation point and waitpid has a hard requirement to be one. But pclose has a hard requirement to not be a cancellation point but it also has "If a thread is canceled during execution of pclose(), the behavior is undefined." The implications of the combination are confusing. > if the function weren't used for the former, it could just always be > cancellable -- wait4 probably should have been cancellable all along, > and almost surely is on other implementations, so to use it safely > you'd have to not use, block, or handle cancellation. I have no immediate objection to making wait4 a cancellation point but I would prefer to make that change consistently on all architectures. I don't have a good sense of who uses cancellation and wait4 and what compatibility constraints are imposed by code (as opposed to standards). > So I'm leaning towards sticking with something like what you've done, > but with declaration moved to src/include/sys/wait.h, or possibly > src/internal/syscall.h (since it's essentially emulation of a I'd be very happy to not have a one-line header file. I looked at syscall.h earlier but wasn't sure if it fit; I did not consider reserved namespace in a public header (would this preclude use of hidden?). Is src/linux/__exact_name_of_function.c the appropriate name for the file? > syscall). Inline in src/internal/syscall.h might also be an option; > one appealing aspect of that is that it would get rid of the #ifdefs > in source files and allow calling __sys_wait4() unconditionally with > no codegen regression on existing archs. This is analogous to > __sys_open. > > Rich -s
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.