yes, timeout: adding pipeline signal handlers #10166
Conversation
|
Somehow every SELinux test failed here |
|
I think I might take the approach of splitting this to first deploy the timeout and yes fixes and then make the change to revert the overrides. |
|
I'm discovering how much the timeout utility has changed between now and the launch of 9.9, its quite different when it comes to signal handling |
ChrisDryden
force-pushed
the
fix-sigpipe-7252
branch
from
b502626 to
ad74ceb
Compare
ChrisDryden
changed the title
|
Getting this working is going to be a very complex issue when it comes to ordering the PR's. I have a working branch that implements timeout in the same way that the latest master in coreutils works, but its involves quite a bunch of changes so I'm hoping to get this one in before I make that PR. |
|
GNU testsuite comparison: |
| if uucore::signals::sigpipe_was_ignored() { | ||
| uucore::show_error!( | ||
| "{}", | ||
| translate!("yes-error-standard-output", "error" => err) |
There was a problem hiding this comment.
If I understand correctly, we should use "err.kind()" instead of "err" in order to match exactly GNU yes error string and remove the "os error 32":
asteba@asteba-MS-7C75:~/dev/coreutils$ trap '' PIPE
asteba@asteba-MS-7C75:~/dev/coreutils$ ./target/debug/coreutils yes | head -1
y
yes: standard output: Broken pipe (os error 32)
asteba@asteba-MS-7C75:~/dev/coreutils$ /usr/bin/gnuyes | head -1
y
gnuyes: standard output: Broken pipe
|
|
||
| /// Configures a Command to preserve SIGPIPE disposition for child processes. | ||
| /// When the parent had SIGPIPE ignored, this ensures the child also has it ignored. | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Please add a comment explaining that Rust's stdlib resets SIGPIPE to SIG_DFL when spawning child processes (Rust source: https://github.com/rust-lang/rust/blob/fe98ddcfcfb6f185dbf4adeaf439d8a756da0273/library/std/src/sys/process/unix/unix.rs#L742 ), unless the -Zon-broken-pipe flag is used. This breaks Unix signal inheritance when the parent has SIGPIPE ignored, which is why preserve_sigpipe_for_child() uses pre_exec() to restore the ignored state before exec.
It is critical context that the rust stdlib restores SIGPIPE to SIG_DFL when calling Command functions. Without this context, I'm concerned that future readers would not understand the role of preserve_sigpipe_for_child().
| #[test] | ||
| #[cfg(unix)] | ||
| #[cfg(feature = "yes")] | ||
| fn test_sigpipe_ignored_preserves_for_child() { |
There was a problem hiding this comment.
| fn test_sigpipe_ignored_preserves_for_child() { | |
| fn test_sigpipe_ignored_preserved_for_child() { |
2 participants
There's a bunch of deviation from our implementation of Yes and Timeout and the gnu version and a long time ago, the gnu tests were patched so that for some tests it used the system yes and timeout. Now that we have proper signal handling implementations for our utilities we can revert this change and also see the gaps in our yes and timeout implementation and the gnu implementation by removing the overrides.
One key area that this will fail is the env-signal-handler test. It was relying on a bunch on non-implemented functionality in timeout that we need to implement for this test to pass. Mainly related to the non standard posix signals like RTMAX/RTMIN which is described here: #6218
UPDATE:
There are still issues with our implementation of timeout, so even though this brings us closer to the GNU implementation, its still not enough to remove the GNU patch. Will handle that in a later PR once timeout is in a better state.