From ec4cc3db93ba66e9d3a0606d605887a8c1750d70 Mon Sep 17 00:00:00 2001 From: mmc Date: Wed, 5 Aug 2026 20:23:07 -0500 Subject: [PATCH 1/2] chcpu: report each error only once Every failure was printed twice: `CpuList::run` reported it itself with a raw `eprintln!`, then also returned it, so `uucore` printed it again with the `chcpu: ` prefix. The unprefixed copy came first, which is the one users saw. Reporting each per-CPU failure with `uucore::show!` keeps the diagnostic for every element of a CPU list, gives all of them the program-name prefix that was previously only on the duplicate, and sets the exit code rather than returning an error - the pattern uucore documents for non-fatal errors when applying an operation to many items. Exit codes are untouched: 1 when nothing succeeded, 64 for partial success. Adds the first tests for this utility, covering the error paths. They need no privileges and change no CPU state: an absent CPU index is rejected before anything is written, and enabling an already-enabled CPU only prints. --- src/uu/chcpu/src/chcpu.rs | 43 +++++++++++++------------ tests/by-util/test_chcpu.rs | 64 +++++++++++++++++++++++++++++++++++++ tests/tests.rs | 4 +++ 3 files changed, 90 insertions(+), 21 deletions(-) create mode 100644 tests/by-util/test_chcpu.rs diff --git a/src/uu/chcpu/src/chcpu.rs b/src/uu/chcpu/src/chcpu.rs index 810f5a86..4e9c9425 100644 --- a/src/uu/chcpu/src/chcpu.rs +++ b/src/uu/chcpu/src/chcpu.rs @@ -204,30 +204,27 @@ impl fmt::Display for DispatchMode { pub(crate) struct CpuList(RangeInclusiveSet); 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))] diff --git a/tests/by-util/test_chcpu.rs b/tests/by-util/test_chcpu.rs new file mode 100644 index 00000000..e4fb0169 --- /dev/null +++ b/tests/by-util/test_chcpu.rs @@ -0,0 +1,64 @@ +// 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. + +#[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 { + (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")); + } +} diff --git a/tests/tests.rs b/tests/tests.rs index 09ffc245..95682dd3 100644 --- a/tests/tests.rs +++ b/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; From 37dde02dd9a6773e3625bde13a4b9fa04f2efdc1 Mon Sep 17 00:00:00 2001 From: mmc Date: Wed, 5 Aug 2026 20:23:09 -0500 Subject: [PATCH 2/2] chcpu: cover argument parsing in tests These reach only clap, never sysfs, so they also guard the option surface on the platforms where the utility itself is unimplemented. --- tests/by-util/test_chcpu.rs | 64 +++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/by-util/test_chcpu.rs b/tests/by-util/test_chcpu.rs index e4fb0169..4c5913ae 100644 --- a/tests/by-util/test_chcpu.rs +++ b/tests/by-util/test_chcpu.rs @@ -3,6 +3,70 @@ // 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 ' cannot be used with '--disable '", + ); +} + +#[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;