stty: report write errors instead of panicking (#10552) - #13252
PranavRJoshi wants to merge 2 commits into
Conversation
Merging this PR will improve performance by 4.81%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | sort_ascii_utf8_locale |
16.1 ms | 15.4 ms | +4.81% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing PranavRJoshi:stty-10552 (50935c4) with main (eae191c)
Footnotes
-
46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
GNU testsuite comparison: |
|
We generally define error kinds in an enum using #[derive(Debug, Error)]
pub enum SttyError {
#[error("{}: {}", translate!("stty-error-write"), strip_errno(.0))]
Write(io::Error),
}Then maybe we wouldn't need the write!(stdout(), "{text}").map_err(SttyError::Write)?; |
|
Done. Refactored into thiserror enum. |
|
@cakebaker PTAL |
|
@PranavRJoshi needs to be rebased |
stty panicked when writing to stdout failed because all output went through the 'print!' family of macros, which panics on I/O errors. Replace the macros with explicit writes to stdout and propagate failures as UResult, mapping them with a localized "write error" so behavior matches GNU stty with exit code 1. Functions previously returning nix::Result now returns UResult as nix errors convert through the existing From impl, so their behavior is unchanged. Lastly, opt out of uucore's exit-time stdout flush since stty now reports its own write errors and every output path ends with a newline. The extra flush would otherwise report the same error twice. Signed-off-by: PranavRJoshi <pranavrjoshi1@gmail.com>
Signed-off-by: PranavRJoshi <pranavrjoshi1@gmail.com>
50935c4 to
ed3c8d5
Compare
|
@xtqqczze done. Let me know if there are other changes needed. |
| stty-error-invalid-argument = invalid argument '{$arg}' | ||
| stty-error-invalid-integer-argument = invalid integer argument: {$value} | ||
| stty-error-invalid-integer-argument-value-too-large = invalid integer argument: {$value}: Value too large for defined data type | ||
| stty-error-write-error = write error |
There was a problem hiding this comment.
we don't need this string, we can use common-write-error instead
| use std::cmp::Ordering; | ||
| use std::fs::File; | ||
| use std::io::{self, Stdin, stdin, stdout}; | ||
| use std::io::{self, Stdin, Write, stdin, stdout}; |
There was a problem hiding this comment.
| use std::io::{self, Stdin, Write, stdin, stdout}; | |
| use std::io::{self, Stdin, Write as _, stdin, stdout}; |
Traits imported that aren’t used directly can be imported anonymously with use Trait as _.
| } | ||
|
|
||
| fn print_special_setting(setting: &PrintSetting, fd: i32) -> nix::Result<()> { | ||
| fn print_special_setting(setting: &PrintSetting, fd: i32) -> UResult<()> { |
There was a problem hiding this comment.
| fn print_special_setting(setting: &PrintSetting, fd: i32) -> UResult<()> { | |
| fn print_special_setting(setting: &PrintSetting, fd: i32) -> Result<(), SttyError> { |
change return type to be more specific where possible
This branch has not been deployed
2 participants
Fixes #10552.
stty > /dev/fullpanicked because allsttyoutput went through theprint!family of macros, which panics when the underlying write tostdoutfails.GNU
sttyreports the error and fails gracefully.Changes:
print!/println!usage insttywith explicit writes tostdoutby introducing a small helper functionwrite_stdout.WrappedPrinter::print/flushand theprint_*functions now returnUResult<()>. Functions that previously returnednix::Resultpropagate their nix errors through the existingFromimpl, so behavior of ioctl/termios failures is unchanged.main.rstouucore::bin!(uu_stty, no_flush), similar to whatcksumalready does.sttynow handles its ownstdoutwrite errors and every output path ends with a newline. This also prevents reporting the same error twice.stty-error-write-errorto US and FR locales.test_write_error_to_dev_full, covering default,--all,--save, andsizeoutput paths.