-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
refactor(cp): replace println! with write_stdout_line for robust error #9901
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
base: main
Are you sure you want to change the base?
Conversation
…r handling Introduce a new `write_stdout_line` function that locks stdout and returns a Result to handle write failures gracefully. Replace all `println!` calls in verbose output, debug messages, and path printing with this function, propagating errors via `CopyResult<()>` instead of panicking on I/O errors. This improves reliability when stdout is closed or encounters issues during cp operations.
|
GNU testsuite comparison: |
…justing exit code - Made `code` mutable to allow modification on flush errors. - Ignore BrokenPipe errors during stdout flush to avoid breaking utilities that silence it (e.g., seq). - Set exit code to 1 if a flush error occurs and the original code was 0, treating write errors as failures.
|
GNU testsuite comparison: |
- Added check to capture if stdout was closed before utility execution using fcntl on Unix. - Modified flush error handling to ignore EBADF errors when stdout was pre-closed, avoiding false failures for silent commands.
Reorder imports for consistency and refactor the ignore_closed_stdout condition to a more concise form, improving code readability without altering functionality.
Add `stdout_is_closed()` and `is_closed_stdout_error()` functions to check if stdout is closed and detect write errors due to closed stdout. Refactor the `bin!` macro to use these functions, reducing code duplication and improving reusability across utilities.
|
GNU testsuite comparison: |
Implement mechanism to detect if stdout was closed at process start and track write attempts. Add `check_stdout_write()` to error on writes when stdout is closed, preventing issues in utilities like cp and printf. Use `TrackingWriter` in printf for write monitoring. This improves robustness in scenarios like pipeline redirections or closed stdout.
|
GNU testsuite comparison: |
…lity - Condensed multi-line cfg_attr into a single line in early_stdout_state mod - Split long variable assignment across lines in bin macro for better readability This improves code consistency and readability without changing functionality.
|
GNU testsuite comparison: |
|
OK. Even so, the function you added to uucore looks helpful for them. |
| /// This function handles non-UTF-8 environment variable names and values correctly by using | ||
| /// raw bytes on Unix systems. | ||
| pub fn print_all_env_vars<T: fmt::Display>(line_ending: T) -> io::Result<()> { | ||
| crate::check_stdout_write(1)?; |
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.
please add a comment explaining why this line
it isn't obvious
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.
the comment doesn't help me to understand why you are adding this here
Add a check to ensure stdout is writable before printing environment variables, preventing errors when stdout is closed. This uses a non-zero sentinel to trigger the check and mark write attempt.
src/uu/mktemp/src/mktemp.rs
Outdated
| }; | ||
| println_verbatim(res?).map_err_context(|| translate!("mktemp-error-failed-print")) | ||
|
|
||
| let path = res?; |
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.
seems unrelated, please move that into a different PR
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.
fix
|
GNU testsuite comparison: |
- Updated the comment to better explain that the path writes via OsWrite, bypassing TrackingWriter, thus requiring an early check to mark stdout as "written" and fail if fd 1 is already closed. This improves code readability and understanding of the stdout-closed handling logic.
Introduce a new
write_stdout_linefunction that locks stdout and returns a Result to handle write failures gracefully. Replace allprintln!calls in verbose output, debug messages, and path printing with this function, propagating errors viaCopyResult<()>instead of panicking on I/O errors. This improves reliability when stdout is closed or encounters issues during cp operations.related
misc/close-stdout.sh