From 6ef08d7f1c48af230c6c8d86e3b8c7ba935bae36 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 19 Apr 2024 01:20:06 +0200 Subject: [PATCH] hashsum: improve the error management to match GNU Should make tests/cksum/md5sum.pl and tests/cksum/sha1sum.pl pass --- src/uu/hashsum/src/hashsum.rs | 80 ++++++++++++++++++----- src/uucore/src/lib/macros.rs | 8 +++ tests/by-util/test_hashsum.rs | 115 +++++++++++++++++++++++++++++++++- util/build-gnu.sh | 3 - 4 files changed, 184 insertions(+), 22 deletions(-) diff --git a/src/uu/hashsum/src/hashsum.rs b/src/uu/hashsum/src/hashsum.rs index 597bca565..d4c5aacbb 100644 --- a/src/uu/hashsum/src/hashsum.rs +++ b/src/uu/hashsum/src/hashsum.rs @@ -26,7 +26,8 @@ use uucore::sum::{ Blake2b, Blake3, Digest, DigestWriter, Md5, Sha1, Sha224, Sha256, Sha384, Sha3_224, Sha3_256, Sha3_384, Sha3_512, Sha512, Shake128, Shake256, }; -use uucore::{display::Quotable, show_warning}; +use uucore::util_name; +use uucore::{display::Quotable, show_warning_caps}; use uucore::{format_usage, help_about, help_usage}; const NAME: &str = "hashsum"; @@ -577,7 +578,6 @@ fn uu_app(binary_name: &str) -> Command { #[derive(Debug)] enum HashsumError { InvalidRegex, - InvalidFormat, IgnoreNotCheck, } @@ -588,7 +588,6 @@ impl std::fmt::Display for HashsumError { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { match self { Self::InvalidRegex => write!(f, "invalid regular expression"), - Self::InvalidFormat => Ok(()), Self::IgnoreNotCheck => write!( f, "the --ignore-missing option is meaningful only when verifying checksums" @@ -644,8 +643,10 @@ where I: Iterator, { let mut bad_format = 0; + let mut correct_format = 0; let mut failed_cksum = 0; let mut failed_open_file = 0; + let mut skip_summary = false; let binary_marker = if options.binary { "*" } else { " " }; for filename in files { let filename = Path::new(filename); @@ -680,13 +681,14 @@ where let mut gnu_re = gnu_re_template(&bytes_marker, r"(?P[ \*])?")?; let bsd_re = Regex::new(&format!( // it can start with \ - r"^(|\\){algorithm} \((?P.*)\) = (?P[a-fA-F0-9]{digest_size})", + r"^(\\)?{algorithm}\s*\((?P.*)\)\s*=\s*(?P[a-fA-F0-9]{digest_size})$", algorithm = options.algoname, digest_size = bytes_marker, )) .map_err(|_| HashsumError::InvalidRegex)?; let buffer = file; + // iterate on the lines of the file for (i, maybe_line) in buffer.lines().enumerate() { let line = match maybe_line { Ok(l) => l, @@ -701,6 +703,7 @@ where handle_captures(&caps, &bytes_marker, &mut bsd_reversed, &mut gnu_re)? } None => match bsd_re.captures(&line) { + // if the GNU style parsing failed, try the BSD style Some(caps) => ( caps.name("fileName").unwrap().as_str().to_string(), caps.name("digest").unwrap().as_str().to_ascii_lowercase(), @@ -709,11 +712,14 @@ where None => { bad_format += 1; if options.strict { - return Err(HashsumError::InvalidFormat.into()); + // if we use strict, the warning "lines are improperly formatted" + // will trigger an exit code of 1 + set_exit_code(1); } if options.warn { - show_warning!( - "{}: {}: improperly formatted {} checksum line", + eprintln!( + "{}: {}: {}: improperly formatted {} checksum line", + util_name(), filename.maybe_quote(), i + 1, options.algoname @@ -727,7 +733,8 @@ where let f = match File::open(ck_filename_unescaped) { Err(_) => { if options.ignore_missing { - // No need to show or return an error. + // No need to show or return an error + // except when the file doesn't have any successful checks continue; } @@ -762,6 +769,7 @@ where // and display it using uucore::display::print_verbatim(). This is // easier (and more important) on Unix than on Windows. if sum == real_sum { + correct_format += 1; if !options.quiet { println!("{prefix}{ck_filename}: OK"); } @@ -770,6 +778,7 @@ where println!("{prefix}{ck_filename}: FAILED"); } failed_cksum += 1; + set_exit_code(1); } } } else { @@ -795,20 +804,57 @@ where println!("{}{} {}{}", prefix, sum, binary_marker, escaped_filename); } } + if bad_format > 0 && failed_cksum == 0 && correct_format == 0 && !options.status { + // we have only bad format. we didn't have anything correct. + // GNU has a different error message for this (with the filename) + set_exit_code(1); + eprintln!( + "{}: {}: no properly formatted checksum lines found", + util_name(), + filename.maybe_quote(), + ); + skip_summary = true; + } + if options.ignore_missing && correct_format == 0 { + // we have only bad format + // and we had ignore-missing + eprintln!( + "{}: {}: no file was verified", + util_name(), + filename.maybe_quote(), + ); + skip_summary = true; + set_exit_code(1); + } } - if !options.status { + + if !options.status && !skip_summary { match bad_format.cmp(&1) { - Ordering::Equal => show_warning!("{} line is improperly formatted", bad_format), - Ordering::Greater => show_warning!("{} lines are improperly formatted", bad_format), + Ordering::Equal => { + show_warning_caps!("{} line is improperly formatted", bad_format) + } + Ordering::Greater => { + show_warning_caps!("{} lines are improperly formatted", bad_format) + } Ordering::Less => {} }; - if failed_cksum > 0 { - show_warning!("{} computed checksum did NOT match", failed_cksum); - } - match failed_open_file.cmp(&1) { - Ordering::Equal => show_warning!("{} listed file could not be read", failed_open_file), + + match failed_cksum.cmp(&1) { + Ordering::Equal => { + show_warning_caps!("{} computed checksum did NOT match", failed_cksum) + } Ordering::Greater => { - show_warning!("{} listed files could not be read", failed_open_file); + show_warning_caps!("{} computed checksums did NOT match", failed_cksum) + } + Ordering::Less => {} + }; + + match failed_open_file.cmp(&1) { + Ordering::Equal => { + show_warning_caps!("{} listed file could not be read", failed_open_file) + } + Ordering::Greater => { + show_warning_caps!("{} listed files could not be read", failed_open_file); } Ordering::Less => {} } diff --git a/src/uucore/src/lib/macros.rs b/src/uucore/src/lib/macros.rs index d1a09c281..359c9d00b 100644 --- a/src/uucore/src/lib/macros.rs +++ b/src/uucore/src/lib/macros.rs @@ -182,6 +182,14 @@ macro_rules! show_warning( }) ); +#[macro_export] +macro_rules! show_warning_caps( + ($($args:tt)+) => ({ + eprint!("{}: WARNING: ", $crate::util_name()); + eprintln!($($args)+); + }) +); + /// Display an error and [`std::process::exit`] /// /// Displays the provided error message using [`show_error!`], then invokes diff --git a/tests/by-util/test_hashsum.rs b/tests/by-util/test_hashsum.rs index 535a4d0ba..432eb3ddf 100644 --- a/tests/by-util/test_hashsum.rs +++ b/tests/by-util/test_hashsum.rs @@ -244,7 +244,7 @@ fn test_check_file_not_found_warning() { .arg(at.subdir.join("testf.sha1")) .fails() .stdout_is("sha1sum: testf: No such file or directory\ntestf: FAILED open or read\n") - .stderr_is("sha1sum: warning: 1 listed file could not be read\n"); + .stderr_is("sha1sum: WARNING: 1 listed file could not be read\n"); } // Asterisk `*` is a reserved paths character on win32, nor the path can end with a whitespace. @@ -471,7 +471,7 @@ fn test_check_empty_line() { .ccmd("md5sum") .arg("--check") .arg(at.subdir.join("in.md5")) - .fails() + .succeeds() .stderr_contains("WARNING: 1 line is improperly formatted"); } @@ -498,3 +498,114 @@ fn test_check_with_escape_filename() { .succeeds(); result.stdout_is("\\a\\nb: OK\n"); } + +#[test] +fn test_check_strict_error() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("f", ""); + at.write( + "in.md5", + "ERR\nERR\nd41d8cd98f00b204e9800998ecf8427e f\nERR\n", + ); + scene + .ccmd("md5sum") + .arg("--check") + .arg("--strict") + .arg(at.subdir.join("in.md5")) + .fails() + .stderr_contains("WARNING: 3 lines are improperly formatted"); +} + +#[test] +fn test_check_warn() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("f", ""); + at.write( + "in.md5", + "d41d8cd98f00b204e9800998ecf8427e f\nd41d8cd98f00b204e9800998ecf8427e f\ninvalid\n", + ); + scene + .ccmd("md5sum") + .arg("--check") + .arg("--warn") + .arg(at.subdir.join("in.md5")) + .succeeds() + .stderr_contains("in.md5: 3: improperly formatted MD5 checksum line") + .stderr_contains("WARNING: 1 line is improperly formatted"); + + // with strict, we should fail the execution + scene + .ccmd("md5sum") + .arg("--check") + .arg("--strict") + .arg(at.subdir.join("in.md5")) + .fails(); +} + +#[test] +fn test_check_status() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("f", ""); + at.write("in.md5", "MD5(f)= d41d8cd98f00b204e9800998ecf8427f\n"); + scene + .ccmd("md5sum") + .arg("--check") + .arg("--status") + .arg(at.subdir.join("in.md5")) + .fails() + .no_output(); +} + +#[test] +fn test_check_status_code() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("f", ""); + at.write("in.md5", "d41d8cd98f00b204e9800998ecf8427f f\n"); + scene + .ccmd("md5sum") + .arg("--check") + .arg("--status") + .arg(at.subdir.join("in.md5")) + .fails() + .stderr_is("") + .stdout_is(""); +} + +#[test] +fn test_check_no_backslash_no_space() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("f", ""); + at.write("in.md5", "MD5(f)= d41d8cd98f00b204e9800998ecf8427e\n"); + scene + .ccmd("md5sum") + .arg("--check") + .arg(at.subdir.join("in.md5")) + .succeeds() + .stdout_is("f: OK\n"); +} + +#[test] +fn test_check_check_ignore_no_file() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + + at.write("f", ""); + at.write("in.md5", "d41d8cd98f00b204e9800998ecf8427f missing\n"); + scene + .ccmd("md5sum") + .arg("--check") + .arg("--ignore-missing") + .arg(at.subdir.join("in.md5")) + .fails() + .stderr_contains("in.md5: no file was verified"); +} diff --git a/util/build-gnu.sh b/util/build-gnu.sh index e8095fe6c..debb7bdd9 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -337,9 +337,6 @@ ls: invalid --time-style argument 'XX'\nPossible values are: [\"full-iso\", \"lo # "hostid BEFORE --help AFTER " same for this sed -i -e "s/env \$prog \$BEFORE \$opt > out2/env \$prog \$BEFORE \$opt > out2 #/" -e "s/env \$prog \$BEFORE \$opt AFTER > out3/env \$prog \$BEFORE \$opt AFTER > out3 #/" -e "s/compare exp out2/compare exp out2 #/" -e "s/compare exp out3/compare exp out3 #/" tests/help/help-version-getopt.sh -# The case doesn't really matter here -sed -i -e "s|WARNING: 1 line is improperly formatted|warning: 1 line is improperly formatted|" tests/cksum/md5sum-bsd.sh - # Add debug info + we have less syscall then GNU's. Adjust our check. # Use GNU sed for /c command "${SED}" -i -e '/test \$n_stat1 = \$n_stat2 \\/c\