Add Unix.sigwait and implement Thread signal-related functions with Unix#13429
Add Unix.sigwait and implement Thread signal-related functions with Unix#13429xavierleroy merged 5 commits intoocaml:trunkfrom
Unix.sigwait and implement Thread signal-related functions with Unix#13429Conversation
gasche
left a comment
There was a problem hiding this comment.
Looks fine to me, thanks! Approved.
|
|
||
| decode_sigset(sigs, &set); | ||
| caml_enter_blocking_section(); | ||
| retcode = sigwait(&set, &signo); |
There was a problem hiding this comment.
Note: in st_posix.h, the use of this function was conditioned on the HAS_SIGWAIT macro, correesponding to a AC_CHECK_FUNC([sigwait]...) check in the configure. Now it is only conditioned on the POSIX_SIGNALS macro, but the definition of POSIX_SIGNALS only checks for sigaction and sigprocmask. So now we are (newly) assuming that if those two are available, then sigwait also is (and, above, sigsuspend).
There was a problem hiding this comment.
See my helpful commit message in b62ba96. This kind of per-function configure test was appropriate in the 1990's, but nowadays either you're on Windows or you have full POSIX.1-2008 (or at the very least -2001) support.
otherlibs/unix/signals.c
Outdated
| caml_enter_blocking_section(); | ||
| retcode = sigwait(&set, &signo); | ||
| caml_leave_blocking_section(); | ||
| if (retcode == -1) caml_uerror("sigwait", Nothing); |
There was a problem hiding this comment.
This logic is wrong. sigwait either returns zero or a positive errno.
| if (retcode == -1) caml_uerror("sigwait", Nothing); | |
| if (retcode != 0) caml_uerror("sigwait", Nothing); |
There was a problem hiding this comment.
Well spotted, thanks. The correct fix uses caml_unix_error, see b62ba96
otherlibs/unix/unixLabels.mli
Outdated
|
|
||
| val sigprocmask : mode:sigprocmask_command -> int list -> int list | ||
| (** [sigprocmask ~mode sigs] changes the set of blocked signals. | ||
| (** [sigprocmask mode sigs] changes the set of blocked signals. |
There was a problem hiding this comment.
Why did you remove the tilde? It's a labelled argument.
There was a problem hiding this comment.
It was a cut-and-paste error, obviously. Fixed.
In preparation for subsuming `Thread.wait_signal`.
The signal mask is per thread and per domain.
It used to be that `Unix.sigprocmask` calls `sigprocmask` while `Thread.sigmask` calls `pthread_sigmask`. However, since OCaml 5, `Unix.sigprocmask` also calls `pthread_sigmask`, so the separate implementation in `Thread.sigmask` is no longer needed. Likewise, the previous commit introduced `Unix.sigwait`, with exactly the same functionality as `Thread.wait_signal`.
`sigwait` is POSIX.1-2001. If we have POSIX signals, we have `sigwait`.
5817afb to
5d42dc2
Compare
Also: bow to check-typo.
|
Thanks for the reviews and the approvals. I feel brave enough to merge. |
The
sigwaitsystem call is currently exposed in the Thread interface, but it's more generally useful, so add it toUnix.As explained in the individual commits,
Thread.sigmaskis now functionally equivalent toUnix.sigprocmask, andThread.wait_signalto the newUnix.sigwait, so we can implement theThreadfunctions in terms of theUnixfunctions, getting rid of some stub code in otherlibs/systhreads.This will also help with #13348 .