PR: 278459
Details
Details
- Reviewers
brooks emaste markj olce - Commits
- rGfb2ab7ce52d3: Add test for pthread_sigqueue(3)
rG91da6becae62: pthread_sigqueue(3): document
rG220aa0f45040: libthr: add pthread_sigqueue(3)
rG97acb35bd704: sys/signal.h: move union sigval into dedicated private header sys/_sigval.h
rGdcc180c51b9c: sigqueue(2): Document __SIGQUEUE_TID
rG53186bc1435e: sigqueue(2): add impl-specific flag __SIGQUEUE_TID
rG0c11c1792b13: kern_thr.c: normalize includes
rG2effad53b467: kern_thr.c/kern_sig.c: remove sys/cdefs.h
rG53e0938b0b56: kern_thread.c: remove unneeded include of sys/param.h
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This review adds pthread_sigqueue(3) as requested. Right now it is not tested and unfinished: it lacks man pages update and test case, which I add after the issue below is discussed.
As usual, I prefer not to add a new syscall that only differs a small bit from sigqueue(2), where we need to be able to directly designate a thread in the current process. One option is to silently allow to pass tids from the current process, but since one of the supported use cases for sigqueue(2) seems to be a process liveness check for sig=0 (same as for kill(2)), I decided not to. Instead, two high bits in the signum are carved for flags, one meaning the tid from curproc is allowed, and the other is reserved for future. This still gives small ABI incompatibility, but the typical wrong use of signum by passing large invalid signal numbers should be still catched.
Comment Actions
Not creating a new syscall saves a bit of work, but apart from that I mostly see disadvantages. The interface would be (slightly) cluttered, and then has to be maintained almost indefinitely. Although the situation is arguably different, this is what was done for rtprio(), which was bent to accept 0 for the current thread. This didn't prevent the need for a later rtprio_thread(), and just increases user's cognitive burden overall. Moreover, it shouldn't be hard nor that time-consuming to factorize the common code parts. While using high bits of signum is understandable (we'll never have that number of signals), it is still conflation of matters, and of a different kind than passing 0 to mean the current thread in some other APIs expecting PIDs. Finally, passing a negative signal is certainly a mistake, but then the user would get a strange error code in this case. So I'd really prefer that we avoid doing this kind of things again. What's the problem with one more syscall ?
| include/pthread.h | ||
|---|---|---|
| 306 ↗ | (On Diff #137392) | Presumably this should appear at the beginning of the file? |
| sys/kern/kern_thr.c | ||
| 44 ↗ | (On Diff #137392) | This sorts before rwlock.h. |
| sys/sys/_sigval.h | ||
| 53 ↗ | (On Diff #137392) | Probably want __uint32_t here. |
| sys/sys/signal.h | ||
| 458 ↗ | (On Diff #137392) | Some comment would be nice: /* The target is a thread ID, not a process ID. */ |
| sys/sys/_sigval.h | ||
|---|---|---|
| 53 ↗ | (On Diff #137392) | The header does not need to be self-contained since it is not supposed to be included explicitly even by the libc or kernel. But I changed that anyway. |
Comment Actions
Having one (or more) syscalls that differ only in small semantic details makes the overall kernel/user interface much more hard to grasp. The cost of having slightly less direct KBI in the layer between libc (libsys) and kernel is very well worth it when consumer of the interface does not need to suffer from dozen of similar but slightly different ways to do the same thing.
Comment Actions
Handle Mark' notes:
- move include of sys/_sigval.h at the top of pthread.h
- add comments to the signo flags definitions
- use _types.h names for types
-sort rwlock.h in kern_thr.c
Comment Actions
I'd lean toward a syscall myself, but don't feel strongly.
| sys/sys/signal.h | ||
|---|---|---|
| 460 ↗ | (On Diff #137400) | I'm a little worried about using the sign bit have surprising effects, but it's probably ok. |
Comment Actions
Yes, but I'd argue there is a balance to find between too many syscalls that clutter the interface and overloading existing ones too much, for cases that differ from the original one. Here, in particular, sigqueue() is a function of the POSIX interface (which doesn't mean it has to correspond 1:1 to a syscall, but the syscall name creates confusion).
I'd still prefer a separate syscall, but I think an acceptable alternative, much less hacky/ugly than the current situation of passing __SIGQUEUE_TID in signum, would just be to allow a thread ID in pid. After all, this argument is to pass the target (so there's arguably no real change in semantics, so no real reason to create a new syscall), the underlying type for PIDs and thread IDs is the same and their numerical ranges are disjoint (and this conflation is already performed by other syscalls). You ruled out this possibility according to your initial comment on the ground of the existence test, but I don't see how this really matters (this test could be performed on threads as well; this is exactly what the code in kern_sigqueue() above does, after the new argument unpacking). What would be the problem here?
Going to remove the Herald rule adding me as a blocking reviewer. That said, I would find it a bit sad that this goes in as is.
| lib/libthr/tests/pthread_sigqueue_test.c | ||
|---|---|---|
| 1 ↗ | (On Diff #137449) | Missing SPDX identifier. |
| share/man/man3/pthread_sigqueue.3 | ||
| 1 ↗ | (On Diff #137449) | The license preamble is missing. |
| 51 ↗ | (On Diff #137449) | |
| 55 ↗ | (On Diff #137449) | Or .Nm is similar to |
| 77 ↗ | (On Diff #137449) | I suppose the EAGAIN error from sigqueue(2) also applies here. |
| 88 ↗ | (On Diff #137449) | |
| 90 ↗ | (On Diff #137449) | Extra newline. |