chcpu: report each error only once

src/uu/chcpu/src/chcpu.rs

@@ -204,30 +204,27 @@ impl fmt::Display for DispatchMode {
 pub(crate) struct CpuList(RangeInclusiveSet<usize>);
 
 impl CpuList {
-    fn run(&self, f: &mut dyn FnMut(usize) -> Result<(), ChCpuError>) -> Result<(), ChCpuError> {
+    /// A failure on one CPU must not stop the remaining ones, so failures are
+    /// reported here and reflected in the exit code instead of being returned:
+    /// returning one would let `uucore` print it a second time.
+    fn run(&self, f: &mut dyn FnMut(usize) -> Result<(), ChCpuError>) {
         use std::ops::RangeInclusive;
 
-        let iter = self.0.iter().flat_map(RangeInclusive::to_owned).map(f);
+        let mut success_occurred = false;
+        let mut failure_occurred = false;
 
-        let (success_occurred, first_error) =
-            iter.fold((false, None), |(success_occurred, first_error), result| {
-                if let Err(err) = result {
-                    eprintln!("{err}");
-                    (success_occurred, first_error.or(Some(err)))
-                } else {
-                    (true, first_error)
+        for cpu_index in self.0.iter().flat_map(RangeInclusive::to_owned) {
+            match f(cpu_index) {
+                Ok(()) => success_occurred = true,
+                Err(err) => {
+                    uucore::show!(err);
+                    failure_occurred = true;
                 }
-            });
-
-        if let Some(err) = first_error {
-            if success_occurred {
-                uucore::error::set_exit_code(64); // Partial success.
-                Ok(())
-            } else {
-                Err(err)
             }
-        } else {
-            Ok(())
+        }
+
+        if success_occurred && failure_occurred {
+            uucore::error::set_exit_code(64); // Partial success.
         }
     }
 }
@@ -293,7 +290,9 @@ fn enable_cpu(cpu_list: &CpuList, enable: bool) -> Result<(), ChCpuError> {
 
     cpu_list.run(&mut move |cpu_index| {
         sysfs_cpu.enable_cpu(enabled_cpu_list.as_mut(), cpu_index, enable)
-    })
+    });
+
+    Ok(())
 }
 
 #[cfg(not(unix))]
@@ -309,7 +308,9 @@ fn configure_cpu(cpu_list: &CpuList, configure: bool) -> Result<(), ChCpuError>
 
     cpu_list.run(&mut move |cpu_index| {
         sysfs_cpu.configure_cpu(enabled_cpu_list.as_ref(), cpu_index, configure)
-    })
+    });
+
+    Ok(())
 }
 
 #[cfg(not(unix))]

tests/by-util/test_chcpu.rs

@@ -0,0 +1,128 @@
+// This file is part of the uutils util-linux package.
+//
+// For the full copyright and license information, please view the LICENSE
+// file that was distributed with this source code.
+
+use uutests::new_ucmd;
+
+#[test]
+fn test_invalid_arg() {
+    new_ucmd!().arg("--definitely-invalid").fails().code_is(1);
+}
+
+#[test]
+fn test_no_args_shows_usage() {
+    new_ucmd!()
+        .fails()
+        .code_is(1)
+        .stderr_contains("configure CPUs in a multi-processor system.");
+}
+
+#[test]
+fn test_actions_mutually_exclusive() {
+    new_ucmd!()
+        .args(&["--enable", "0", "--disable", "1"])
+        .fails()
+        .code_is(1)
+        .stderr_contains(
+            "the argument '--enable <cpu-list>' cannot be used with '--disable <cpu-list>'",
+        );
+}
+
+#[test]
+fn test_cpu_list_range_out_of_order() {
+    new_ucmd!()
+        .args(&["--enable", "3-1"])
+        .fails()
+        .code_is(1)
+        .stderr_contains("first element of CPU list range is greater than its last element");
+}
+
+#[test]
+fn test_cpu_list_not_a_number() {
+    new_ucmd!()
+        .args(&["--enable", "a"])
+        .fails()
+        .code_is(1)
+        .stderr_contains("CPU list element is not a positive number");
+}
+
+/// An empty argument splits into one empty element rather than zero elements, so it
+/// is rejected as an unparsable element; `ChCpuError::EmptyCpuList` is unreachable.
+#[test]
+fn test_cpu_list_empty() {
+    new_ucmd!()
+        .args(&["--enable", ""])
+        .fails()
+        .code_is(1)
+        .stderr_contains("CPU list element is not a positive number");
+}
+
+#[test]
+fn test_dispatch_mode_unknown() {
+    new_ucmd!()
+        .args(&["--dispatch", "bogus"])
+        .fails()
+        .code_is(1)
+        .stderr_contains("[possible values: horizontal, vertical]");
+}
+
+#[cfg(target_os = "linux")]
+mod linux {
+    use uutests::new_ucmd;
+
+    /// CPU indices no kernel can have: `CONFIG_NR_CPUS` is orders of magnitude below
+    /// these, so `/sys/devices/system/cpu/cpu9999[89]` never exists and `chcpu`
+    /// rejects them before it would write anything.
+    const ABSENT_CPU: &str = "99999";
+    const ABSENT_CPU_2: &str = "99998";
+
+    /// First CPU exposing an `online` attribute that reads `1`, or `None` where no
+    /// CPU is hot-pluggable. `cpu0` commonly has no such attribute, so a CPU index
+    /// cannot simply be assumed.
+    fn first_online_cpu() -> Option<usize> {
+        (0..1024).find(|index| {
+            std::fs::read_to_string(format!("/sys/devices/system/cpu/cpu{index}/online"))
+                .is_ok_and(|state| state.trim() == "1")
+        })
+    }
+
+    #[test]
+    fn test_absent_cpu_is_reported_once() {
+        new_ucmd!()
+            .arg("--enable")
+            .arg(ABSENT_CPU)
+            .fails_with_code(1)
+            .stderr_only(format!("chcpu: CPU {ABSENT_CPU} does not exist\n"));
+    }
+
+    #[test]
+    fn test_every_absent_cpu_is_reported_once() {
+        new_ucmd!()
+            .arg("--enable")
+            .arg(format!("{ABSENT_CPU_2},{ABSENT_CPU}"))
+            .fails_with_code(1)
+            .stderr_only(format!(
+                "chcpu: CPU {ABSENT_CPU_2} does not exist\nchcpu: CPU {ABSENT_CPU} does not exist\n"
+            ));
+    }
+
+    /// A list mixing a usable CPU with an absent one must still exit 64 (partial
+    /// success) and report the failure once. Enabling an already-enabled CPU returns
+    /// before writing, so no privileges are needed and no CPU state changes, barring
+    /// someone racing the test by offlining that CPU between the two reads.
+    #[test]
+    fn test_partial_success_reports_failure_once() {
+        let Some(cpu) = first_online_cpu() else {
+            eprintln!("skipping test_partial_success_reports_failure_once: no hot-pluggable CPU");
+            return;
+        };
+
+        new_ucmd!()
+            .arg("--enable")
+            .arg(format!("{cpu},{ABSENT_CPU}"))
+            .fails_with_code(64)
+            .stdout_is(format!("CPU {cpu} is already enabled\n"))
+            .stderr_is(format!("chcpu: CPU {ABSENT_CPU} does not exist\n"));
+    }
+}

tests/tests.rs

@@ -94,3 +94,7 @@ mod test_mcookie;
 #[cfg(feature = "uuidgen")]
 #[path = "by-util/test_uuidgen.rs"]
 mod test_uuidgen;
+
+#[cfg(feature = "chcpu")]
+#[path = "by-util/test_chcpu.rs"]
+mod test_chcpu;