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

@@ -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<FileInfo> {
 }
 
 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)]

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<usize> = 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::<usize>().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),
+    }
+}

src/uu/od/src/parse_nrofbytes.rs

@@ -1,4 +1,6 @@
-pub fn parse_number_of_bytes(s: &str) -> Result<usize, &'static str> {
+use uucore::parse_size::{parse_size, ParseSizeError};
+
+pub fn parse_number_of_bytes(s: &str) -> Result<usize, ParseSizeError> {
     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<usize, &'static str> {
     } 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<usize, &'static str> {
                 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<usize, &'static str> {
-    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());
 }

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<OsStr>) -> Box<dyn Read + Send> {
     }
 }
 
+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),

src/uu/stdbuf/src/stdbuf.rs

@@ -118,13 +118,10 @@ fn check_option(matches: &ArgMatches, name: &str) -> Result<BufferType, ProgramO
                     Ok(BufferType::Line)
                 }
             }
-            x => {
-                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),
     }

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")

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));
+    }
+}

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
+                ));
         }
     }
 }

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");
+    }
 }