Skip to content

Conversation

@mattsu2020
Copy link
Contributor

@mattsu2020 mattsu2020 commented Dec 29, 2025

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.

related
misc/close-stdout.sh

…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.
@github-actions
Copy link

GNU testsuite comparison:

Note: The gnu test tests/id/smack was skipped on 'main' but is now failing.
Note: The gnu test tests/mkdir/smack-no-root was skipped on 'main' but is now failing.
Note: The gnu test tests/mkdir/smack-root was skipped on 'main' but is now failing.

…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.
@github-actions
Copy link

GNU testsuite comparison:

Note: The gnu test tests/basenc/bounded-memory is now being skipped but was previously passing.

- 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.
@mattsu2020 mattsu2020 marked this pull request as draft December 29, 2025 04:59
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.
@github-actions
Copy link

GNU testsuite comparison:

Note: The gnu test tests/id/smack was skipped on 'main' but is now failing.
Note: The gnu test tests/mkdir/smack-no-root was skipped on 'main' but is now failing.
Note: The gnu test tests/mkdir/smack-root was skipped on 'main' but is now failing.

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.
@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/tty/tty-eof. tests/tty/tty-eof is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/misc/close-stdout is no longer failing!

…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.
@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/close-stdout is no longer failing!

@oech3
Copy link
Contributor

oech3 commented Dec 29, 2025

I don't hope to fixing everyng to keep size of PR small, but is it related with #9769 #9795?

@mattsu2020
Copy link
Contributor Author

I don't hole to fixing everyng to keep size of PR small, but is it related with #9769 #9795?

#9769
#9795
Both issues need to be fixed separately.

@mattsu2020 mattsu2020 marked this pull request as ready for review December 29, 2025 09:16
@oech3
Copy link
Contributor

oech3 commented Dec 29, 2025

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)?;
Copy link
Contributor

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

Copy link
Contributor

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

mattsu2020 and others added 2 commits December 29, 2025 20:07
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.
};
println_verbatim(res?).map_err_context(|| translate!("mktemp-error-failed-print"))

let path = res?;
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

@github-actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/misc/close-stdout is no longer failing!

- 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.
@oech3

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants