From db3ee61742138e7bc1dd14bb3a1bd67db418f5a5 Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Thu, 3 Jun 2021 21:00:03 +0200 Subject: [PATCH] du/sort/od/stdbuf: make error handling of SIZE/BYTES/MODE arguments more consistent * od: add stderr info for not yet implemented '--strings' flag --- src/uu/du/src/du.rs | 31 ++++++++------- src/uu/od/src/od.rs | 65 +++++++++++++++++++------------- src/uu/od/src/parse_nrofbytes.rs | 45 +++++++++++----------- src/uu/sort/src/sort.rs | 24 ++++++++---- src/uu/stdbuf/src/stdbuf.rs | 11 ++---- tests/by-util/test_du.rs | 2 +- tests/by-util/test_od.rs | 34 +++++++++++++++++ tests/by-util/test_sort.rs | 9 +++-- tests/by-util/test_stdbuf.rs | 25 ++++++------ 9 files changed, 153 insertions(+), 93 deletions(-) diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 5dfb41d82..170e057b5 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -42,7 +42,7 @@ mod options { pub const NULL: &str = "0"; pub const ALL: &str = "all"; pub const APPARENT_SIZE: &str = "apparent-size"; - pub const BLOCK_SIZE: &str = "B"; + pub const BLOCK_SIZE: &str = "block-size"; pub const BYTES: &str = "b"; pub const TOTAL: &str = "c"; pub const MAX_DEPTH: &str = "d"; @@ -211,18 +211,11 @@ fn get_file_info(path: &PathBuf) -> Option { } fn read_block_size(s: Option<&str>) -> usize { - if let Some(size_arg) = s { - match parse_size(size_arg) { - Ok(v) => v, - Err(e) => match e { - ParseSizeError::ParseFailure(_) => { - crash!(1, "invalid suffix in --block-size argument '{}'", size_arg) - } - ParseSizeError::SizeTooBig(_) => { - crash!(1, "--block-size argument '{}' too large", size_arg) - } - }, - } + if let Some(s) = s { + parse_size(s).map_or_else( + |e| crash!(1, "{}", format_error_message(e, s, options::BLOCK_SIZE)), + |n| n, + ) } else { for env_var in &["DU_BLOCK_SIZE", "BLOCK_SIZE", "BLOCKSIZE"] { if let Ok(env_size) = env::var(env_var) { @@ -389,7 +382,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .arg( Arg::with_name(options::BLOCK_SIZE) .short("B") - .long("block-size") + .long(options::BLOCK_SIZE) .value_name("SIZE") .help( "scale sizes by SIZE before printing them. \ @@ -694,6 +687,16 @@ Try '{} --help' for more information.", 0 } +fn format_error_message(error: ParseSizeError, s: &str, option: &str) -> String { + // NOTE: + // GNU's du echos affected flag, -B or --block-size (-t or --threshold), depending user's selection + // GNU's du distinguishs between "invalid (suffix in) argument" + match error { + ParseSizeError::ParseFailure(_) => format!("invalid --{} argument '{}'", option, s), + ParseSizeError::SizeTooBig(_) => format!("--{} argument '{}' too large", option, s), + } +} + #[cfg(test)] mod test_du { #[allow(unused_imports)] diff --git a/src/uu/od/src/od.rs b/src/uu/od/src/od.rs index 7359047b2..9b98fd515 100644 --- a/src/uu/od/src/od.rs +++ b/src/uu/od/src/od.rs @@ -43,6 +43,7 @@ use crate::partialreader::*; use crate::peekreader::*; use crate::prn_char::format_ascii_dump; use clap::{self, AppSettings, Arg, ArgMatches}; +use uucore::parse_size::ParseSizeError; use uucore::InvalidEncodingHandling; static VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -130,15 +131,16 @@ impl OdOptions { } }; - let mut skip_bytes = match matches.value_of(options::SKIP_BYTES) { - None => 0, - Some(s) => match parse_number_of_bytes(&s) { - Ok(i) => i, - Err(_) => { - return Err(format!("Invalid argument --skip-bytes={}", s)); - } - }, - }; + if matches.occurrences_of(options::STRINGS) > 0 { + crash!(1, "Option '{}' not yet implemented.", options::STRINGS); + } + + let mut skip_bytes = matches.value_of(options::SKIP_BYTES).map_or(0, |s| { + parse_number_of_bytes(s).map_or_else( + |e| crash!(1, "{}", format_error_message(e, s, options::SKIP_BYTES)), + |n| n, + ) + }); let mut label: Option = None; @@ -161,11 +163,16 @@ impl OdOptions { } }; - let mut line_bytes = match matches.value_of(options::WIDTH) { - None => 16, - Some(_) if matches.occurrences_of(options::WIDTH) == 0 => 16, - Some(s) => s.parse::().unwrap_or(0), - }; + let mut line_bytes = matches.value_of(options::WIDTH).map_or(16, |s| { + if matches.occurrences_of(options::WIDTH) == 0 { + return 16; + }; + parse_number_of_bytes(s).map_or_else( + |e| crash!(1, "{}", format_error_message(e, s, options::WIDTH)), + |n| n, + ) + }); + let min_bytes = formats.iter().fold(1, |max, next| { cmp::max(max, next.formatter_item_info.byte_size) }); @@ -176,15 +183,12 @@ impl OdOptions { let output_duplicates = matches.is_present(options::OUTPUT_DUPLICATES); - let read_bytes = match matches.value_of(options::READ_BYTES) { - None => None, - Some(s) => match parse_number_of_bytes(&s) { - Ok(i) => Some(i), - Err(_) => { - return Err(format!("Invalid argument --read-bytes={}", s)); - } - }, - }; + let read_bytes = matches.value_of(options::READ_BYTES).map(|s| { + parse_number_of_bytes(s).map_or_else( + |e| crash!(1, "{}", format_error_message(e, s, options::READ_BYTES)), + |n| n, + ) + }); let radix = match matches.value_of(options::ADDRESS_RADIX) { None => Radix::Octal, @@ -265,7 +269,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .short("S") .long(options::STRINGS) .help( - "output strings of at least BYTES graphic chars. 3 is assumed when \ + "NotImplemented: output strings of at least BYTES graphic chars. 3 is assumed when \ BYTES is not specified.", ) .default_value("3") @@ -466,8 +470,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let od_options = match OdOptions::new(clap_matches, args) { Err(s) => { - show_usage_error!("{}", s); - return 1; + crash!(1, "{}", s); } Ok(o) => o, }; @@ -649,3 +652,13 @@ fn open_input_peek_reader( let pr = PartialReader::new(mf, skip_bytes, read_bytes); PeekReader::new(pr) } + +fn format_error_message(error: ParseSizeError, s: &str, option: &str) -> String { + // NOTE: + // GNU's od echos affected flag, -N or --read-bytes (-j or --skip-bytes, etc.), depending user's selection + // GNU's od distinguishs between "invalid (suffix in) argument" + match error { + ParseSizeError::ParseFailure(_) => format!("invalid --{} argument '{}'", option, s), + ParseSizeError::SizeTooBig(_) => format!("--{} argument '{}' too large", option, s), + } +} diff --git a/src/uu/od/src/parse_nrofbytes.rs b/src/uu/od/src/parse_nrofbytes.rs index 9223d7e53..e51e15d39 100644 --- a/src/uu/od/src/parse_nrofbytes.rs +++ b/src/uu/od/src/parse_nrofbytes.rs @@ -1,4 +1,6 @@ -pub fn parse_number_of_bytes(s: &str) -> Result { +use uucore::parse_size::{parse_size, ParseSizeError}; + +pub fn parse_number_of_bytes(s: &str) -> Result { let mut start = 0; let mut len = s.len(); let mut radix = 16; @@ -9,10 +11,7 @@ pub fn parse_number_of_bytes(s: &str) -> Result { } else if s.starts_with('0') { radix = 8; } else { - return match uucore::parse_size::parse_size(&s[start..]) { - Ok(n) => Ok(n), - Err(_) => Err("parse failed"), - }; + return parse_size(&s[start..]); } let mut ends_with = s.chars().rev(); @@ -60,35 +59,33 @@ pub fn parse_number_of_bytes(s: &str) -> Result { Some('P') => 1000 * 1000 * 1000 * 1000 * 1000, #[cfg(target_pointer_width = "64")] Some('E') => 1000 * 1000 * 1000 * 1000 * 1000 * 1000, - _ => return Err("parse failed"), + _ => return Err(ParseSizeError::ParseFailure(s.to_string())), } } _ => {} } - match usize::from_str_radix(&s[start..len], radix) { - Ok(i) => Ok(i * multiply), - Err(_) => Err("parse failed"), - } -} - -#[allow(dead_code)] -fn parse_number_of_bytes_str(s: &str) -> Result { - parse_number_of_bytes(&String::from(s)) + let factor = match usize::from_str_radix(&s[start..len], radix) { + Ok(f) => f, + Err(e) => return Err(ParseSizeError::ParseFailure(e.to_string())), + }; + factor + .checked_mul(multiply) + .ok_or(ParseSizeError::SizeTooBig(s.to_string())) } #[test] fn test_parse_number_of_bytes() { // octal input - assert_eq!(8, parse_number_of_bytes_str("010").unwrap()); - assert_eq!(8 * 512, parse_number_of_bytes_str("010b").unwrap()); - assert_eq!(8 * 1024, parse_number_of_bytes_str("010k").unwrap()); - assert_eq!(8 * 1048576, parse_number_of_bytes_str("010m").unwrap()); + assert_eq!(8, parse_number_of_bytes("010").unwrap()); + assert_eq!(8 * 512, parse_number_of_bytes("010b").unwrap()); + assert_eq!(8 * 1024, parse_number_of_bytes("010k").unwrap()); + assert_eq!(8 * 1048576, parse_number_of_bytes("010m").unwrap()); // hex input - assert_eq!(15, parse_number_of_bytes_str("0xf").unwrap()); - assert_eq!(15, parse_number_of_bytes_str("0XF").unwrap()); - assert_eq!(27, parse_number_of_bytes_str("0x1b").unwrap()); - assert_eq!(16 * 1024, parse_number_of_bytes_str("0x10k").unwrap()); - assert_eq!(16 * 1048576, parse_number_of_bytes_str("0x10m").unwrap()); + assert_eq!(15, parse_number_of_bytes("0xf").unwrap()); + assert_eq!(15, parse_number_of_bytes("0XF").unwrap()); + assert_eq!(27, parse_number_of_bytes("0x1b").unwrap()); + assert_eq!(16 * 1024, parse_number_of_bytes("0x10k").unwrap()); + assert_eq!(16 * 1048576, parse_number_of_bytes("0x10m").unwrap()); } diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 208010d09..c148d06c7 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -1148,12 +1148,12 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.buffer_size = matches .value_of(OPT_BUF_SIZE) - .map(|v| match GlobalSettings::parse_byte_count(v) { - Ok(n) => n, - Err(ParseSizeError::ParseFailure(_)) => crash!(2, "invalid -S argument '{}'", v), - Err(ParseSizeError::SizeTooBig(_)) => crash!(2, "-S argument '{}' too large", v), - }) - .unwrap_or(DEFAULT_BUF_SIZE); + .map_or(DEFAULT_BUF_SIZE, |s| { + GlobalSettings::parse_byte_count(s).map_or_else( + |e| crash!(2, "{}", format_error_message(e, s, OPT_BUF_SIZE)), + |n| n, + ) + }); settings.tmp_dir = matches .value_of(OPT_TMP_DIR) @@ -1555,6 +1555,16 @@ fn open(path: impl AsRef) -> Box { } } +fn format_error_message(error: ParseSizeError, s: &str, option: &str) -> String { + // NOTE: + // GNU's sort echos affected flag, -S or --buffer-size, depending user's selection + // GNU's sort distinguishs between "invalid (suffix in) argument" + match error { + ParseSizeError::ParseFailure(_) => format!("invalid --{} argument '{}'", option, s), + ParseSizeError::SizeTooBig(_) => format!("--{} argument '{}' too large", option, s), + } +} + #[cfg(test)] mod tests { @@ -1659,7 +1669,7 @@ mod tests { ("10T", 10 * 1024 * 1024 * 1024 * 1024), ("1b", 1), ("1024b", 1024), - ("1024Mb", 1024 * 1024 * 1024), // TODO: This might not be what GNU `sort` does? + ("1024Mb", 1024 * 1024 * 1024), // NOTE: This might not be how GNU `sort` behaves for 'Mb' ("1", 1024), // K is default ("50", 50 * 1024), ("K", 1024), diff --git a/src/uu/stdbuf/src/stdbuf.rs b/src/uu/stdbuf/src/stdbuf.rs index e39af3816..d073c1bd1 100644 --- a/src/uu/stdbuf/src/stdbuf.rs +++ b/src/uu/stdbuf/src/stdbuf.rs @@ -118,13 +118,10 @@ fn check_option(matches: &ArgMatches, name: &str) -> Result { - let size = match parse_size(x) { - Ok(m) => m, - Err(e) => return Err(ProgramOptionsError(format!("invalid mode {}", e))), - }; - Ok(BufferType::Size(size)) - } + x => parse_size(x).map_or_else( + |e| crash!(125, "invalid mode {}", e), + |m| Ok(BufferType::Size(m)), + ), }, None => Ok(BufferType::Default), } diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index 8ec0a9c0c..066ab5c9b 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -80,7 +80,7 @@ fn test_du_invalid_size() { .arg("/tmp") .fails() .code_is(1) - .stderr_only("du: invalid suffix in --block-size argument '1fb4t'"); + .stderr_only("du: invalid --block-size argument '1fb4t'"); #[cfg(not(target_pointer_width = "128"))] new_ucmd!() .arg("--block-size=1Y") diff --git a/tests/by-util/test_od.rs b/tests/by-util/test_od.rs index c21c683dc..3d34e47a1 100644 --- a/tests/by-util/test_od.rs +++ b/tests/by-util/test_od.rs @@ -804,3 +804,37 @@ fn test_traditional_only_label() { ", )); } + +#[test] +fn test_od_invalid_bytes() { + const INVALID_SIZE: &str = "1fb4t"; + const BIG_SIZE: &str = "1Y"; + + let input: [u8; 8] = [0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]; + + let options = [ + "--read-bytes", + "--skip-bytes", + "--width", + // "--strings", // TODO: consider testing here once '--strings' is implemented + ]; + for option in &options { + new_ucmd!() + .arg(format!("{}={}", option, INVALID_SIZE)) + .run_piped_stdin(&input[..]) + .failure() + .code_is(1) + .stderr_only(format!( + "od: invalid {} argument '{}'", + option, INVALID_SIZE + )); + + #[cfg(not(target_pointer_width = "128"))] + new_ucmd!() + .arg(format!("{}={}", option, BIG_SIZE)) + .run_piped_stdin(&input[..]) + .failure() + .code_is(1) + .stderr_only(format!("od: {} argument '{}' too large", option, BIG_SIZE)); + } +} diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 054789edf..eab256980 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -57,7 +57,7 @@ fn test_invalid_buffer_size() { .fails() .code_is(2) .stderr_only(format!( - "sort: invalid -S argument '{}'", + "sort: invalid --buffer-size argument '{}'", invalid_buffer_size )); } @@ -69,7 +69,7 @@ fn test_invalid_buffer_size() { .arg("ext_sort.txt") .fails() .code_is(2) - .stderr_only("sort: -S argument '1Y' too large"); + .stderr_only("sort: --buffer-size argument '1Y' too large"); #[cfg(target_pointer_width = "32")] { @@ -82,7 +82,10 @@ fn test_invalid_buffer_size() { .arg("ext_sort.txt") .fails() .code_is(2) - .stderr_only(format!("sort: -S argument '{}' too large", buffer_size)); + .stderr_only(format!( + "sort: --buffer-size argument '{}' too large", + buffer_size + )); } } } diff --git a/tests/by-util/test_stdbuf.rs b/tests/by-util/test_stdbuf.rs index e5d784edb..fc1b9324a 100644 --- a/tests/by-util/test_stdbuf.rs +++ b/tests/by-util/test_stdbuf.rs @@ -57,15 +57,18 @@ fn test_stdbuf_line_buffering_stdin_fails() { #[cfg(not(target_os = "windows"))] #[test] fn test_stdbuf_invalid_mode_fails() { - // TODO: GNU's `stdbuf` (8.32) does not return "\nTry 'stdbuf --help' for more information." - // for invalid modes. - new_ucmd!() - .args(&["-i", "1024R", "head"]) - .fails() - .stderr_contains("stdbuf: invalid mode ‘1024R’"); - #[cfg(not(target_pointer_width = "128"))] - new_ucmd!() - .args(&["--error", "1Y", "head"]) - .fails() - .stderr_contains("stdbuf: invalid mode ‘1Y’: Value too large to be stored in data type"); + let options = ["--input", "--output", "--error"]; + for option in &options { + new_ucmd!() + .args(&[*option, "1024R", "head"]) + .fails() + .code_is(125) + .stderr_only("stdbuf: invalid mode ‘1024R’"); + #[cfg(not(target_pointer_width = "128"))] + new_ucmd!() + .args(&[*option, "1Y", "head"]) + .fails() + .code_is(125) + .stderr_contains("stdbuf: invalid mode ‘1Y’: Value too large for defined data type"); + } }