[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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[23].
>
> 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.
