-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
stdbuf: use exec instead of forking #9495
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
stdbuf: use exec instead of forking #9495
Conversation
|
GNU testsuite comparison: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
de2b66c to
0110357
Compare
|
GNU testsuite comparison: |
Merging this PR will not alter performance
Comparing Footnotes
|
|
GNU testsuite comparison: |
0d7529b to
4c557bf
Compare
|
GNU testsuite comparison: |
|
This is ready for review. I don't intend to work on this branch any more. |
|
I've just seen #9013 (comment) which is also relevant for my PR. I'll check if |
4c557bf to
9afe842
Compare
|
GNU testsuite comparison: |
9afe842 to
af3957e
Compare
|
GNU testsuite comparison: |
|
The PR is ready for review. |
af3957e to
197203c
Compare
|
GNU testsuite comparison: |
197203c to
8ae82a8
Compare
|
GNU testsuite comparison: |
8ae82a8 to
11a9c80
Compare
|
GNU testsuite comparison: |
11a9c80 to
7963769
Compare
|
GNU testsuite comparison: |
8534afe to
964753c
Compare
|
GNU testsuite comparison: |
Forking creates a new PID and it not compatible with GNU coreutils implementation. - use exec instead of forking - add test verifying that execvp is used Fixes uutils#9066 Signed-off-by: Etienne Cordonnier <[email protected]>
964753c to
a235f1a
Compare
|
GNU testsuite comparison: |
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.
Pull request overview
This PR replaces the fork+spawn implementation in stdbuf with a direct exec() call, making it compatible with GNU coreutils. The previous implementation broke signal handling and PR_SET_PDEATHSIG because it created a new PID through forking. With exec(), the stdbuf process is replaced by the target command, preserving the PID and allowing signals to propagate correctly.
Changes:
- Replaced
Command::spawn()withCommand::exec()to avoid forking - Simplified error handling by removing wait/signal logic (no longer needed with exec)
- Added comprehensive test to verify no fork occurs by checking /proc cmdline
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/uu/stdbuf/src/stdbuf.rs | Replaces spawn+wait with exec, updates imports, simplifies error handling to match exec-only flow |
| tests/by-util/test_stdbuf.rs | Adds Linux-specific regression test that validates exec behavior by polling /proc to ensure process replacement occurs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
@cakebaker can you please review this PR? |
forking creates a new PID and it not compatible with GNU coreutils implementation.
Command::exec()instead ofCommand::spawn()which forksFixes #9066