From c1d81b687cce1ffcaadf40f83b67160f23735942 Mon Sep 17 00:00:00 2001
From: NotAShelf <raf@notashelf.dev>
Date: Fri, 16 May 2025 03:03:35 +0300
Subject: [PATCH] battery:  deduplicate frequency validation logic

---
 src/battery.rs | 35 ++++++++++++++++++++---------------
 src/main.rs    | 46 ++++++++++++++++++++++++----------------------
 2 files changed, 44 insertions(+), 37 deletions(-)

diff --git a/src/battery.rs b/src/battery.rs
index a3c3d3f..d555bd5 100644
--- a/src/battery.rs
+++ b/src/battery.rs
@@ -164,14 +164,16 @@ fn write_sysfs_value(path: impl AsRef<Path>, value: &str) -> Result<()> {
 /// Read a value from a sysfs file
 fn read_sysfs_value(path: impl AsRef<Path>) -> Result<String> {
     let p = path.as_ref();
-    fs::read_to_string(p).map_err(|e| {
-        let error_msg = format!("Path: {:?}, Error: {}", p.display(), e);
-        if e.kind() == io::ErrorKind::PermissionDenied {
-            ControlError::PermissionDenied(error_msg)
-        } else {
-            ControlError::ReadError(error_msg)
-        }
-    }).map(|s| s.trim().to_string())
+    fs::read_to_string(p)
+        .map_err(|e| {
+            let error_msg = format!("Path: {:?}, Error: {}", p.display(), e);
+            if e.kind() == io::ErrorKind::PermissionDenied {
+                ControlError::PermissionDenied(error_msg)
+            } else {
+                ControlError::ReadError(error_msg)
+            }
+        })
+        .map(|s| s.trim().to_string())
 }
 
 /// Safely check if a path exists and is writable
@@ -216,18 +218,18 @@ fn apply_thresholds_to_batteries(
     for battery in batteries {
         let start_path = battery.path.join(battery.pattern.start_path);
         let stop_path = battery.path.join(battery.pattern.stop_path);
-        
+
         // Read current thresholds in case we need to restore them
         let current_start = read_sysfs_value(&start_path).ok();
         let current_stop = read_sysfs_value(&stop_path).ok();
-        
+
         // Write stop threshold first (must be >= start threshold)
         let stop_result = write_sysfs_value(&stop_path, &stop_threshold.to_string());
-        
+
         // Only proceed to set start threshold if stop threshold was set successfully
-        if let Ok(()) = stop_result {
+        if matches!(stop_result, Ok(())) {
             let start_result = write_sysfs_value(&start_path, &start_threshold.to_string());
-            
+
             match start_result {
                 Ok(()) => {
                     debug!(
@@ -246,10 +248,13 @@ fn apply_thresholds_to_batteries(
                                 battery.name, re
                             );
                         } else {
-                            debug!("Restored previous stop threshold ({}) for battery '{}'", prev_stop, battery.name);
+                            debug!(
+                                "Restored previous stop threshold ({}) for battery '{}'",
+                                prev_stop, battery.name
+                            );
                         }
                     }
-                    
+
                     errors.push(format!(
                         "Failed to set start threshold for {} battery '{}': {}",
                         battery.pattern.description, battery.name, e
diff --git a/src/main.rs b/src/main.rs
index de2cbcc..471b18e 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -358,17 +358,9 @@ fn main() {
         }
         Some(Commands::SetMinFreq { freq_mhz, core_id }) => {
             // Basic validation for reasonable CPU frequency values
-            if freq_mhz == 0 {
-                error!("Minimum frequency cannot be zero");
-                Err(Box::new(ControlError::InvalidValueError(
-                    "Minimum frequency cannot be zero".to_string(),
-                )) as Box<dyn std::error::Error>)
-            } else if freq_mhz > 10000 {
-                // Extremely high value unlikely to be valid
-                error!("Minimum frequency ({freq_mhz} MHz) is unreasonably high");
-                Err(Box::new(ControlError::InvalidValueError(format!(
-                    "Minimum frequency ({freq_mhz} MHz) is unreasonably high"
-                ))) as Box<dyn std::error::Error>)
+            if let Err(e) = validate_freq(freq_mhz, "Minimum") {
+                error!("{e}");
+                Err(e)
             } else {
                 cpu::set_min_frequency(freq_mhz, core_id)
                     .map_err(|e| Box::new(e) as Box<dyn std::error::Error>)
@@ -376,17 +368,9 @@ fn main() {
         }
         Some(Commands::SetMaxFreq { freq_mhz, core_id }) => {
             // Basic validation for reasonable CPU frequency values
-            if freq_mhz == 0 {
-                error!("Maximum frequency cannot be zero");
-                Err(Box::new(ControlError::InvalidValueError(
-                    "Maximum frequency cannot be zero".to_string(),
-                )) as Box<dyn std::error::Error>)
-            } else if freq_mhz > 10000 {
-                // Extremely high value unlikely to be valid
-                error!("Maximum frequency ({freq_mhz} MHz) is unreasonably high");
-                Err(Box::new(ControlError::InvalidValueError(format!(
-                    "Maximum frequency ({freq_mhz} MHz) is unreasonably high"
-                ))) as Box<dyn std::error::Error>)
+            if let Err(e) = validate_freq(freq_mhz, "Maximum") {
+                error!("{e}");
+                Err(e)
             } else {
                 cpu::set_max_frequency(freq_mhz, core_id)
                     .map_err(|e| Box::new(e) as Box<dyn std::error::Error>)
@@ -497,3 +481,21 @@ fn init_logger() {
         debug!("Logger initialized with RUST_LOG={env_log}");
     });
 }
+
+/// Validate CPU frequency input values
+fn validate_freq(freq_mhz: u32, label: &str) -> Result<(), Box<dyn std::error::Error>> {
+    if freq_mhz == 0 {
+        error!("{label} frequency cannot be zero");
+        Err(Box::new(ControlError::InvalidValueError(format!(
+            "{label} frequency cannot be zero"
+        ))) as Box<dyn std::error::Error>)
+    } else if freq_mhz > 10000 {
+        // Extremely high value unlikely to be valid
+        error!("{label} frequency ({freq_mhz} MHz) is unreasonably high");
+        Err(Box::new(ControlError::InvalidValueError(format!(
+            "{label} frequency ({freq_mhz} MHz) is unreasonably high"
+        ))) as Box<dyn std::error::Error>)
+    } else {
+        Ok(())
+    }
+}