From 9a67393c444891f989949cb02005ccf9b33af2d4 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Wed, 30 Aug 2023 17:46:09 +0200 Subject: [PATCH] factor: short circuit on write error, but not on parse error --- src/uu/factor/src/cli.rs | 57 ++++++++++++++++-------------------- tests/by-util/test_factor.rs | 31 ++++++++++++++++++++ 2 files changed, 56 insertions(+), 32 deletions(-) diff --git a/src/uu/factor/src/cli.rs b/src/uu/factor/src/cli.rs index bfc4ede15cb..63a0632a3b7 100644 --- a/src/uu/factor/src/cli.rs +++ b/src/uu/factor/src/cli.rs @@ -3,8 +3,6 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -use std::error::Error; -use std::fmt::Write as FmtWrite; use std::io::BufRead; use std::io::{self, stdin, stdout, Write}; @@ -12,7 +10,7 @@ mod factor; use clap::{crate_version, Arg, ArgAction, Command}; pub use factor::*; use uucore::display::Quotable; -use uucore::error::UResult; +use uucore::error::{set_exit_code, FromIo, UResult}; use uucore::{format_usage, help_about, help_usage, show_error, show_warning}; mod miller_rabin; @@ -32,26 +30,27 @@ mod options { fn print_factors_str( num_str: &str, w: &mut io::BufWriter, - factors_buffer: &mut String, print_exponents: bool, -) -> Result<(), Box> { - num_str - .trim() - .parse::() - .map_err(|e| e.into()) - .and_then(|x| { - factors_buffer.clear(); - // If print_exponents is true, use the alternate format specifier {:#} from fmt to print the factors - // of x in the form of p^e. - if print_exponents { - writeln!(factors_buffer, "{}:{:#}", x, factor(x))?; - } else { - writeln!(factors_buffer, "{}:{}", x, factor(x))?; - } - w.write_all(factors_buffer.as_bytes())?; - w.flush()?; - Ok(()) - }) +) -> io::Result<()> { + let x = match num_str.trim().parse::() { + Ok(x) => x, + Err(e) => { + // We return Ok() instead of Err(), because it's non-fatal and we should try the next + // number. + show_warning!("{}: {}", num_str.maybe_quote(), e); + set_exit_code(1); + return Ok(()); + } + }; + + // If print_exponents is true, use the alternate format specifier {:#} from fmt to print the factors + // of x in the form of p^e. + if print_exponents { + writeln!(w, "{}:{:#}", x, factor(x))?; + } else { + writeln!(w, "{}:{}", x, factor(x))?; + } + w.flush() } #[uucore::main] @@ -64,25 +63,19 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let stdout = stdout(); // We use a smaller buffer here to pass a gnu test. 4KiB appears to be the default pipe size for bash. let mut w = io::BufWriter::with_capacity(4 * 1024, stdout.lock()); - let mut factors_buffer = String::new(); if let Some(values) = matches.get_many::(options::NUMBER) { for number in values { - if let Err(e) = print_factors_str(number, &mut w, &mut factors_buffer, print_exponents) - { - show_warning!("{}: {}", number.maybe_quote(), e); - } + print_factors_str(number, &mut w, print_exponents) + .map_err_context(|| "write error".into())?; } } else { let stdin = stdin(); let lines = stdin.lock().lines(); for line in lines { for number in line.unwrap().split_whitespace() { - if let Err(e) = - print_factors_str(number, &mut w, &mut factors_buffer, print_exponents) - { - show_warning!("{}: {}", number.maybe_quote(), e); - } + print_factors_str(number, &mut w, print_exponents) + .map_err_context(|| "write error".into())?; } } } diff --git a/tests/by-util/test_factor.rs b/tests/by-util/test_factor.rs index 2a11363b760..3326a1ace71 100644 --- a/tests/by-util/test_factor.rs +++ b/tests/by-util/test_factor.rs @@ -337,6 +337,37 @@ fn test_primes_with_exponents() { .stdout_is(String::from_utf8(output_string.as_bytes().to_owned()).unwrap()); } +#[test] +fn fails_on_invalid_number() { + new_ucmd!().arg("not-a-valid-number").fails(); + new_ucmd!() + .arg("not-a-valid-number") + .arg("12") + .fails() + .stdout_contains("12: 2 2 3"); +} + +#[test] +#[cfg(any(target_os = "linux", target_os = "freebsd", target_os = "netbsd"))] +fn short_circuit_write_error() { + use std::fs::OpenOptions; + + // Check that the error is printed exactly once and factor does not move on + // to the next number when a write error happens. + // + // Note: Technically, GNU prints the error twice, not because it does not + // short circuit the error, but because it always prints the error twice, + // for any number of inputs. That's silly behavior and printing once is + // clearly better. + let f = OpenOptions::new().write(true).open("/dev/full").unwrap(); + new_ucmd!() + .arg("12") + .arg("10") + .set_stdout(f) + .fails() + .stderr_is("factor: write error: No space left on device\n"); +} + const PRIMES_BY_BITS: &[&[u64]] = &[ PRIMES14, PRIMES15, PRIMES16, PRIMES17, PRIMES18, PRIMES19, PRIMES20, PRIMES21, PRIMES22, PRIMES23, PRIMES24, PRIMES25, PRIMES26, PRIMES27, PRIMES28, PRIMES29, PRIMES30, PRIMES31,