tools/build/make_libc_exterr_cat_filenames.sh: stabilize output Sort the list of the source files to ensure that all runs get the same output. kern/kern_exit.c: some style kern/kern_fork/exit.c: organize includes Remove sys/cdefs.h. Remove sys/param.h. Order the sys/*.h includes alphabetically. kern/kern_fork.c: define the exterror category for fork Convert EINVALs in kern_fork.c into EXTERRORs. sys: add AUE_PDRFORK contrib/openbsm: add AUE_PDRFORK sys: add pdrfork(2) lib/libsys, lib/libc: export pdrfork(2) kern/kern_exit.c: define the exterror category for exit/wait Convert EINVALs in kern_exit.c into EXTERRORs. kern/kern_exit.c: extract some helpers from proc_to_reap() kern/kern_exit.c: extract wait6_check_alive() helper Add pdwait(2) lib/libsys, lib/libc: export pdwait Regen for the fork and exit/wait exterror category addition Regen syscall tables after pdfork(2) and pdwait(2) additions
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 69740 Build 66623: arc lint + arc unit
Event Timeline
Comment Actions
Somewhat improve flags validations.
Convert all EINVALs in kern_fork.c into EXTERRORs.
Comment Actions
I do not write the man pages before the interface is agreed upon and stabilized, at very least. After that, I plan to.
Comment Actions
Rather than add a new syscall for pdwait, what would you say to adding another idtype for waitid and wait6? We already have those syscalls, and their existing interface nicely allows for different kinds of ids. We would just need to add a P_PIDFD idtype value. That's also what Linux does.
Comment Actions
The new syscalls mostly work nicely. I've just found a few bugs. In addition to my inline comments, pdwait never returns a pid. It always returns 0. Is that deliberate? Also, it's unclear to me when a child ought to be reaped. I would think that it should be reaped as soon as pdwait or waitpid returns it (though the pid shouldn't be recycled until the process descriptor is closed). However, I'm seeing that the child doesn't get reaped until close(). That is, pdwait() will return the process multiple times, as if WNOWAIT were set. Is that deliberate?
| sys/kern/kern_exit.c | ||
|---|---|---|
| 1522 | Should we add a cap_pdwait_rights right? | |
| 1526 | Unfortunately, our precedents are inconsistent:
Personally, I think EBADF makes more sense. Do you agree? | |
| sys/kern/kern_fork.c | ||
| 222 | This section is weird. It is expecting that the fd argument be a process descriptor referring to the current process. But RFPROCDESC normally means "return a process descriptor". Why would the caller want the kernel to return a process descriptor if he already has one? fork1 will return EINVAL if RFPROCDESC is set and RFPROC isn't. I think sys_pdrfork should do the same. | |
| 240–241 | This check wrongly results in the copyout not being called if the user called pdrfork with RFSPAWN. | |
| sys/kern/syscalls.master | ||
| 3417 | This argument order is the opposite of every other wait() variant. I suggest that status come before options. | |
Comment Actions
These are related. I tried to keep the semantic of reaping only on close(). This has the nice consequences of pdwait() working for each accessor is the fd is shared between them.
Returning pid sounds useless for me. For normal wait*(2) syscalls, it makes sense because they select some target to reap among children, and this is why they must indicate which one was selected. For pdwait(), fd specifies exactly what to operate on, so what is the point of returning pid?
Due to not reaping and not returning pid, reusing waitid(2) for pdwait() was not suitable, the syscalls has quite different semantic.
Please note that I do not insist on this interface for pdwait(2). If the consensus is be to be more like wait(2), i.e. reap (and return the pid, but this is not too important), then I will change the patch accordingly.
| sys/kern/kern_exit.c | ||
|---|---|---|
| 1522 | Perhaps yes. I will do it later. | |
| 1526 | No, please seefd9e09cb2ab07993e, where the commit message provides my opinion on this question, and a85525a5c8b2. The later is unfortunate. but it explains why pdgetpid() is stuck with EBADF. I do not want to repeat this mistake in the new code. | |
| sys/kern/kern_fork.c | ||
| 222 | RFPROCDESC is the internal kernel flag for the stock main, and I am somewhat repurposing it there. Next, when no RFPROC is specified, this means that the current process is modified by the rfork(2) call. What could be a semantic of passing fd to rfork() without RFPROC? Whatever is it, it a. must specify existing process. b. we can only modify the current process (X). So this is why I am checking that fd references curproc. X This might be considered either as an implementation detail, but I would tend to think that this is fundamental on how much we allow remote process to affect other process. Controlling should be done with ptrace(). | |
Comment Actions
Add cap_pdwait_rights.
Make WNOWAIT functional for pdwait(), or rather make defaults to reap the process on pdwait().
Suggested by: asomers
Comment Actions
I had to build an aarch64 VM just to test the RFSPAWN change, but it works now. It probably works on amd64 too, but I can't test it without writing assembly code.
As for the pdwait() reaping behavior, I don't have a strong opinion either way. I suggested that pdwait(WEXITED) should reap the process just like wait4() for consistency's sake.
As for the RFPROCDESC behavior, I think that your proposal of using pdrfork(RFPROCESC) to modify the current process is very unlikely to be useful. I would prefer that to return EINVAL. OTOH, anybody who misuses pdrfork(RFPROCDESC) will currently get EBADF, which isn't so wrong. So you're way should be ok.
And I've found another problem. Currently, combining pdrfork() with waitpid(WEXITED) will reap the process, and make the pid available for reuse. I thought otherwise last Thursday only because my testing was incomplete. This limitation makes the Rust feature impossible to implement bug-free, at least until pdwait() is available. Would it be possible to reserve the pid even after the process has been reaped, until the process descriptor is closed? That needn't be part of this review, of course, but I mention it in case that affects the requirements of pdwait().
| sys/kern/kern_fork.c | ||
|---|---|---|
| 222 | How does one even get a process descriptor for the current process? The only way I know of is to use pdfork(), then have the parent send the process descriptor to the child via sendmsg. All that just so the child can modify its own process, using pdrfork instead of rfork? That doesn't seem useful to me. | |
| sys/sys/procdesc.h | ||
| 132 | ||