mailing list of musl libc
 help / color / mirror / code / Atom feed
From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [musl] [PATCH 07/14] Emulate wait4 using waitid
Date: Thu, 3 Sep 2020 11:49:36 -0400	[thread overview]
Message-ID: <20200903154935.GA3265@brightrain.aerifal.cx> (raw)
In-Reply-To: <20200903112309.102601-8-sorear@fastmail.com>

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
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.

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.

- 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).

- 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
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.

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
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

  parent reply	other threads:[~2020-09-03 15:49 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-03 11:22 [musl] [PATCH 00/14] riscv32 support Stefan O'Rear
2020-09-03 11:22 ` [musl] [PATCH 01/14] Remove ARMSUBARCH relic from configure Stefan O'Rear
2020-09-03 11:22 ` [musl] [PATCH 02/14] time64: Don't make aliases to nonexistent syscalls Stefan O'Rear
2020-09-03 15:56   ` Rich Felker
2020-09-03 19:36     ` Stefan O'Rear
2020-09-03 21:17       ` Rich Felker
2020-09-03 11:22 ` [musl] [PATCH 03/14] time64: Only getrlimit/setrlimit if they exist Stefan O'Rear
2020-09-03 11:22 ` [musl] [PATCH 04/14] time64: Only gettimeofday/settimeofday if exist Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 05/14] Add src/internal/statx.h Stefan O'Rear
2020-09-03 15:39   ` Arnd Bergmann
2020-09-03 15:51     ` Rich Felker
2020-09-03 18:08       ` Arnd Bergmann
2020-09-03 11:23 ` [musl] [PATCH 06/14] Only call fstatat if defined Stefan O'Rear
2020-09-03 16:05   ` Rich Felker
2020-09-04  1:47     ` Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 07/14] Emulate wait4 using waitid Stefan O'Rear
2020-09-03 14:56   ` Stefan O'Rear
2020-09-03 15:36     ` Arnd Bergmann
2020-09-03 15:40       ` Stefan O'Rear
2020-09-03 18:08         ` Arnd Bergmann
2020-09-03 15:49   ` Rich Felker [this message]
2020-09-03 16:25     ` Stefan O'Rear
2020-09-03 16:38       ` Rich Felker
2020-09-03 11:23 ` [musl] [PATCH 08/14] riscv: Fall back to syscall __riscv_flush_icache Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 09/14] riscv32: Target and subtarget detection Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 10/14] riscv32: add arch headers Stefan O'Rear
2020-09-03 15:49   ` Arnd Bergmann
2020-09-03 11:23 ` [musl] [PATCH 11/14] riscv32: Add fenv and math Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 12/14] riscv32: Add dlsym Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 13/14] riscv32: Add jmp_buf and sigreturn Stefan O'Rear
2020-09-03 11:23 ` [musl] [PATCH 14/14] riscv32: Add thread support Stefan O'Rear

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200903154935.GA3265@brightrain.aerifal.cx \
    --to=dalias@libc.org \
    --cc=musl@lists.openwall.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).