Add a convenience method .pid() for WaitStatus. #722
Conversation
test/sys/test_wait.rs
Outdated
| let _m = ::FORK_MTX.lock().expect("Mutex got poisoned by another test"); | ||
|
|
||
| match fork().unwrap() { | ||
| Child => {}, |
There was a problem hiding this comment.
Don't allow the child to return. It's vulnerable to deadlocks in the test harness code. Instead, call _exit(0).
There was a problem hiding this comment.
You mean that the test harness executes the function as a normal function, so the forked process will duplicate what test harness does?
Why _exit and not std::process::exit? The former is unsafe.
There was a problem hiding this comment.
Yeah, the forked process duplicates the entire test harness. But it doesn't duplicate any threads other than the one that called fork. So if one of those other threads held a mutex that is required by this thread during its teardown phase, then this thread will deadlock. The reason to use _exit instead of sys::process::exit is subtler, and it's because _exit doesn't do any C-level teardown. See fork(2).
There was a problem hiding this comment.
You mean the reasons like this: https://stackoverflow.com/questions/5422831/what-is-the-difference-between-using-exit-exit-in-a-conventional-linux-fo
(double buffer flushing or double execution of atexit handlers?)
Why is there no clean way of doing _exit without unsafe constructs if we're exposing fork as a public function?
There was a problem hiding this comment.
Like that, but worse because the parent is a multi-threaded process. In a multi-threaded process, even memory allocation could theoretically deadlock, so until the child execs, it is limited to only calling async-signal-safe functions, which are a pretty restrictive set.
|
This all looks good to me, though you still need to squash. I'll give @Susurrus a day or so to comment before I merge it. |
|
@asomers Can you squash while merging? The GitHub web UI allows it. |
|
@marmistrz I can't squash while merging, because nix doesn't use the GitHub web UI. We use bors instead. bors prevents test failures caused by merge conflicts, but unfortunately can't automatically squash. |
CHANGELOG.md
Outdated
| @@ -12,6 +12,8 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
| ([#672](https://github.com/nix-rust/nix/pull/672)) | |||
| - Added protocol families in `AddressFamily` enum. | |||
| ([#647](https://github.com/nix-rust/nix/pull/647)) | |||
| - Added a convenience method `.pid()` to the `enum WaitStatus` | |||
There was a problem hiding this comment.
Instead "Added the pid() method to WaitStatus for extracting the PID."
src/sys/wait.rs
Outdated
| /// Extracts the PID from the WaitStatus if the status is not StillAlive. | ||
| pub fn pid(&self) -> Option<Pid> { | ||
| use self::WaitStatus::*; | ||
| match self { |
There was a problem hiding this comment.
I'm not certain if this StackOverflow comment is correct, but it would read nicer if you did match *self to get rid of the & on every match arm.
|
Squashed. |
|
bors r+ |
No description provided.