Skip to content

Commit

Permalink
Merge pull request #5225 from tertsdiepraam/factor-short-circuit-writ…
Browse files Browse the repository at this point in the history
…e-error

`factor`: short circuit on write error, but not on parse error
  • Loading branch information
sylvestre authored Sep 1, 2023
2 parents 6012002 + 9a67393 commit 633ae06
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 32 deletions.
57 changes: 25 additions & 32 deletions src/uu/factor/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,14 @@
// 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};

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;
Expand All @@ -32,26 +30,27 @@ mod options {
fn print_factors_str(
num_str: &str,
w: &mut io::BufWriter<impl io::Write>,
factors_buffer: &mut String,
print_exponents: bool,
) -> Result<(), Box<dyn Error>> {
num_str
.trim()
.parse::<u64>()
.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::<u64>() {
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]
Expand All @@ -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::<String>(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())?;
}
}
}
Expand Down
31 changes: 31 additions & 0 deletions tests/by-util/test_factor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 633ae06

Please sign in to comment.