-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
seq: add SIGPIPE handling for GNU compatibility #9657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/uu/seq/src/seq.rs
Outdated
| // The Rust runtime ignores SIGPIPE, but we need to respect the parent's | ||
| // signal disposition for proper pipeline behavior (GNU compatibility). | ||
| #[cfg(unix)] | ||
| if !uucore::signals::sigpipe_was_ignored() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An approach we can do if we decide that we want this for every since function that inits the sigpipe capture is that we can add this logic to the signals handler itself?
|
GNU testsuite comparison: |
ba48f71 to
1dea4ca
Compare
|
GNU testsuite comparison: |
69ad560 to
832ee2f
Compare
|
GNU testsuite comparison: |
|
The tests that appear to be failing appear to be flaky tests, should be good for review now. This one is required for the next PR which is the env flags for ignoring signals |
|
@sylvestre Would it be better if I made this into a bigger PR where I combine use the uucore::init_sigpipe_capture and signals::sigpipe_was_ignored to address all of the issues related to sigpipe in the queue to show it has full GNU compatibility |
| // The Rust runtime ignores SIGPIPE, but we need to respect the parent's | ||
| // signal disposition for proper pipeline behavior (GNU compatibility). | ||
| #[cfg(unix)] | ||
| if !signals::sigpipe_was_ignored() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this is in, I will follow up with an attempt to bring this to the bin handler so that its applied to all utilities. Ideally we don't need to add this for every utility
9cbe553 to
a2d74e5
Compare
a2d74e5 to
421fd6d
Compare
|
GNU testsuite comparison: |
|
I am very excited that this is in, we are going to be able to fix so many outstanding issues now using this now |
This is a culmination of the work done by @mattsu2020 and @Ecordonnier to address the issues with SIGPIPE handling.
The high level issue is that Rust by default overrides the handling of SIGPIPE and ignores it, and theres a bunch of functionality that relies on this in the GNU Coreutils. @mattsu2020 was able to identify a really ingenious approach where he was able to do:
and it is able to hook into a process that runs before the function is started that is able to set the value of an atomic to the state of the SIGPIPE signal and whether the ignore SIGPIPE signal was set.
I took this implementation here: #9184 and turned it into a macro that you can add to the utility that you need to use these flags for and it gives you the ability to set both the enable_pipe_errors and check the state of sigpipe_was_ignored inside your function to return different error codes depending on the error.
I also then went through every single signal related GNU test and tried this approach to see if it would fix the issues and it appears that this approach is able to address every GNU related signal test: #9646