From 74ff3593fb2b64b224f9979902f04fbfa60ba949 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 30 Jul 2026 21:14:00 +0200 Subject: [PATCH] rm: report a verbose write failure once without aborting the removal --- src/uu/rm/src/platform/unix.rs | 29 +++++++++++++++------------ src/uu/rm/src/rm.rs | 28 ++++++++++++++++---------- tests/by-util/test_rm.rs | 36 ++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 23 deletions(-) diff --git a/src/uu/rm/src/platform/unix.rs b/src/uu/rm/src/platform/unix.rs index 63647620c2..67456c3263 100644 --- a/src/uu/rm/src/platform/unix.rs +++ b/src/uu/rm/src/platform/unix.rs @@ -22,7 +22,7 @@ use uucore::translate; use super::super::{ InteractiveMode, Options, is_dir_empty, is_readable_metadata, prompt_descend, remove_file, - show_permission_denied_error, show_removal_error, show_verbose_write_error, + report_verbose_write_error, show_permission_denied_error, show_removal_error, verbose_removed_directory, verbose_removed_file, }; @@ -128,9 +128,8 @@ pub fn safe_remove_file( if let Some(pb) = progress_bar { pb.inc(1); } - Some(show_verbose_write_error(verbose_removed_file( - path, options, - ))) + report_verbose_write_error(verbose_removed_file(path, options)); + Some(false) } Err(e) => { if e.kind() == std::io::ErrorKind::PermissionDenied { @@ -160,9 +159,8 @@ pub fn safe_remove_empty_dir( if let Some(pb) = progress_bar { pb.inc(1); } - Some(show_verbose_write_error(verbose_removed_directory( - path, options, - ))) + report_verbose_write_error(verbose_removed_directory(path, options)); + Some(false) } Err(e) => { let e = @@ -208,7 +206,8 @@ fn handle_permission_denied( return true; } // Successfully removed empty directory - show_verbose_write_error(verbose_removed_directory(entry_path, options)) + report_verbose_write_error(verbose_removed_directory(entry_path, options)); + false } /// Helper to handle unlink operation with error reporting @@ -225,12 +224,12 @@ fn handle_unlink( show_error!("{e}"); true } else { - let result = if is_dir { + report_verbose_write_error(if is_dir { verbose_removed_directory(entry_path, options) } else { verbose_removed_file(entry_path, options) - }; - show_verbose_write_error(result) + }); + false } } @@ -258,7 +257,10 @@ pub fn remove_dir_with_special_cases(path: &Path, options: &Options, error_occur // of the recursion. error_occurred } - Ok(_) => show_verbose_write_error(verbose_removed_directory(path, options)), + Ok(_) => { + report_verbose_write_error(verbose_removed_directory(path, options)); + false + } } } @@ -321,7 +323,8 @@ pub fn safe_remove_dir_recursive( if e.kind() == std::io::ErrorKind::PermissionDenied { // Try to remove the directory directly if it's empty if fs::remove_dir(path).is_ok() { - return show_verbose_write_error(verbose_removed_directory(path, options)); + report_verbose_write_error(verbose_removed_directory(path, options)); + return false; } // If we can't read the directory AND can't remove it, // show permission denied error for GNU compatibility diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 5e2fc6eb5c..b2cc197357 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -18,6 +18,7 @@ use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::PermissionsExt; use std::path::MAIN_SEPARATOR; use std::path::Path; +use std::sync::atomic::{AtomicBool, Ordering}; use thiserror::Error; use uucore::display::Quotable; use uucore::error::{FromIo, UError, UResult, USimpleError, strip_errno}; @@ -87,12 +88,18 @@ fn verbose_removed_directory(path: &Path, options: &Options) -> UResult<()> { Ok(()) } -/// Helper function to report a verbose output write error and return error status -fn show_verbose_write_error(result: UResult<()>) -> bool { - if let Err(e) = &result { +/// Set once a verbose write failure has happened. Like GNU, a broken standard +/// output neither interrupts the removal nor is reported more than once, but it +/// does make `rm` exit with a failure status. +static VERBOSE_WRITE_FAILED: AtomicBool = AtomicBool::new(false); + +/// Helper function to report a verbose output write error, at most once +fn report_verbose_write_error(result: UResult<()>) { + if let Err(e) = result + && !VERBOSE_WRITE_FAILED.swap(true, Ordering::Relaxed) + { show_error!("{e}"); } - result.is_err() } /// Helper function to show error with context and return error status @@ -116,7 +123,10 @@ fn show_permission_denied_error(path: &Path) -> bool { /// Helper function to remove a directory and handle results fn remove_dir_with_feedback(path: &Path, options: &Options) -> bool { match fs::remove_dir(path) { - Ok(_) => show_verbose_write_error(verbose_removed_directory(path, options)), + Ok(_) => { + report_verbose_write_error(verbose_removed_directory(path, options)); + false + } Err(e) => show_removal_error(e, path), } } @@ -315,7 +325,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } } - if remove(&files, &options) { + if remove(&files, &options) || VERBOSE_WRITE_FAILED.load(Ordering::Relaxed) { return Err(1.into()); } @@ -740,9 +750,7 @@ fn remove_dir_recursive( // show another error message as we return from each level // of the recursion. } - Ok(_) => { - error = error || show_verbose_write_error(verbose_removed_directory(path, options)); - } + Ok(_) => report_verbose_write_error(verbose_removed_directory(path, options)), } error @@ -867,7 +875,7 @@ fn remove_file(path: &Path, options: &Options, progress_bar: Option<&ProgressBar // Fallback method for non-Unix, Redox, or when safe traversal is unavailable match fs::remove_file(path) { Ok(_) => { - return show_verbose_write_error(verbose_removed_file(path, options)); + report_verbose_write_error(verbose_removed_file(path, options)); } Err(e) => { if e.kind() == io::ErrorKind::PermissionDenied { diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index f4a57c74be..023f4a6b6b 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -383,7 +383,43 @@ fn test_verbose_write_error_does_not_panic_dir() { .stderr_contains("No space left on device") .stderr_does_not_contain("panicked"); + // A broken stdout must not stop the removal itself. assert!(!at.file_exists(file)); + assert!(!at.dir_exists(dir)); +} + +// A broken standard output must be reported once, not once per removed entry. +#[test] +#[cfg(target_os = "linux")] +fn test_verbose_write_error_reported_once() { + use std::fs::OpenOptions; + + let (at, mut ucmd) = at_and_ucmd!(); + let dir = "test_rm_verbose_write_error_once"; + + at.mkdir(dir); + for name in ["alpha", "bravo", "charlie", "delta"] { + at.touch(format!("{dir}/{name}")); + } + + let dev_full = OpenOptions::new().write(true).open("/dev/full").unwrap(); + + let result = ucmd + .arg("-r") + .arg("-v") + .arg(dir) + .set_stdout(dev_full) + .fails(); + result.code_is(1); + assert_eq!( + result + .stderr_str() + .matches("No space left on device") + .count(), + 1 + ); + + assert!(!at.dir_exists(dir)); } #[test]