Repository: GuillaumeGomez/sysinfo
Pull request #1075 (2023-09-24 15:03:17)

Fix invalid name retrieval for linux CPU and remove frequency, brand and vendor ID from global CPU
Fixes https://github.com/GuillaumeGomez/sysinfo/issues/1073.


Patch:
diff --git a/src/unix/apple/cpu.rs b/src/unix/apple/cpu.rs
index 30df454..8185edd 100644
--- a/src/unix/apple/cpu.rs
+++ b/src/unix/apple/cpu.rs
@@ -323,11 +323,6 @@ pub(crate) fn init_cpus(
         }
         (percentage, cpus.len())
     });
-
-    // We didn't set them above to avoid cloning them unnecessarily.
-    global_cpu.brand = brand;
-    global_cpu.vendor_id = vendor_id;
-    global_cpu.frequency = frequency;
 }
 
 fn get_sysctl_str(s: &[u8]) -> String {
diff --git a/src/unix/freebsd/cpu.rs b/src/unix/freebsd/cpu.rs
index 9d13688..3ddeda1 100644
--- a/src/unix/freebsd/cpu.rs
+++ b/src/unix/freebsd/cpu.rs
@@ -74,7 +74,6 @@ impl CpusWrapper {
                 self.cpus
                     .push(Cpu::new(format!("cpu {pos}"), vendor_id.clone(), frequency));
             }
-            self.global_cpu.vendor_id = vendor_id;
             self.got_cpu_frequency = refresh_kind.frequency();
         } else if refresh_kind.frequency() && !self.got_cpu_frequency {
             for (pos, proc_) in self.cpus.iter_mut().enumerate() {
@@ -87,9 +86,6 @@ impl CpusWrapper {
         if refresh_kind.cpu_usage() {
             self.get_cpu_usage();
         }
-        if refresh_kind.frequency() {
-            self.global_cpu.frequency = self.cpus.get(0).map(|cpu| cpu.frequency).unwrap_or(0);
-        }
     }
 
     fn get_cpu_usage(&mut self) {
diff --git a/src/unix/linux/cpu.rs b/src/unix/linux/cpu.rs
index e73ff29..3575bf0 100644
--- a/src/unix/linux/cpu.rs
+++ b/src/unix/linux/cpu.rs
@@ -2,7 +2,7 @@
 
 #![allow(clippy::too_many_arguments)]
 
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
 use std::fs::File;
 use std::io::{BufRead, BufReader, Read};
 use std::time::Instant;
@@ -72,10 +72,10 @@ impl CpusWrapper {
             .unwrap_or(true);
 
         let first = self.cpus.is_empty();
-        let (vendor_id, brand) = if first {
+        let mut vendors_brands = if first {
             get_vendor_id_and_brand()
         } else {
-            (String::new(), String::new())
+            HashMap::new()
         };
 
         // If the last CPU usage update is too close (less than `MINIMUM_CPU_UPDATE_INTERVAL`),
@@ -127,6 +127,10 @@ impl CpusWrapper {
 
                         let mut parts = line.split(|x| *x == b' ').filter(|s| !s.is_empty());
                         if first {
+                            let (vendor_id, brand) = match vendors_brands.remove(&i) {
+                                Some((vendor_id, brand)) => (vendor_id, brand),
+                                None => (String::new(), String::new()),
+                            };
                             self.cpus.push(Cpu::new_with_values(
                                 to_str!(parts.next().unwrap_or(&[])),
                                 parts.next().map(to_u64).unwrap_or(0),
@@ -140,8 +144,8 @@ impl CpusWrapper {
                                 parts.next().map(to_u64).unwrap_or(0),
                                 parts.next().map(to_u64).unwrap_or(0),
                                 0,
-                                vendor_id.clone(),
-                                brand.clone(),
+                                vendor_id,
+                                brand,
                             ));
                         } else {
                             parts.next(); // we don't want the name again
@@ -188,22 +192,12 @@ impl CpusWrapper {
             }
 
             // `get_cpu_frequency` is very slow, so better run it in parallel.
-            self.global_cpu.frequency = iter_mut(&mut self.cpus)
+            iter_mut(&mut self.cpus)
                 .enumerate()
-                .map(|(pos, proc_)| {
-                    proc_.frequency = get_cpu_frequency(pos);
-                    proc_.frequency
-                })
-                .max()
-                .unwrap_or(0);
+                .for_each(|(pos, proc_)| proc_.frequency = get_cpu_frequency(pos));
 
             self.got_cpu_frequency = true;
         }
-
-        if first {
-            self.global_cpu.vendor_id = vendor_id;
-            self.global_cpu.brand = brand;
-        }
     }
 
     pub(crate) fn get_global_raw_times(&self) -> (u64, u64) {
@@ -715,13 +709,13 @@ fn get_arm_part(implementer: u32, part: u32) -> Option<&'static str> {
 }
 
 /// Returns the brand/vendor string for the first CPU (which should be the same for all CPUs).
-pub(crate) fn get_vendor_id_and_brand() -> (String, String) {
+pub(crate) fn get_vendor_id_and_brand() -> HashMap<usize, (String, String)> {
     let mut s = String::new();
     if File::open("/proc/cpuinfo")
         .and_then(|mut f| f.read_to_string(&mut s))
         .is_err()
     {
-        return (String::new(), String::new());
+        return HashMap::new();
     }
 
     fn get_value(s: &str) -> String {
@@ -740,40 +734,92 @@ pub(crate) fn get_vendor_id_and_brand() -> (String, String) {
             .unwrap_or_default()
     }
 
-    let mut vendor_id = None;
-    let mut brand = None;
-    let mut implementer = None;
-    let mut part = None;
-
-    for it in s.split('\n') {
-        if it.starts_with("vendor_id\t") {
-            vendor_id = Some(get_value(it));
-        } else if it.starts_with("model name\t") {
-            brand = Some(get_value(it));
-        } else if it.starts_with("CPU implementer\t") {
-            implementer = Some(get_hex_value(it));
-        } else if it.starts_with("CPU part\t") {
-            part = Some(get_hex_value(it));
-        } else {
-            continue;
+    #[inline]
+    fn is_new_processor(line: &str) -> bool {
+        line.starts_with("processor\t")
+            || line.starts_with("processor ")
+            || line.starts_with("processor:")
+    }
+
+    #[derive(Default)]
+    struct CpuInfo {
+        index: usize,
+        vendor_id: Option<String>,
+        brand: Option<String>,
+        implementer: Option<u32>,
+        part: Option<u32>,
+    }
+
+    impl CpuInfo {
+        fn has_all_info(&self) -> bool {
+            (self.brand.is_some() && self.vendor_id.is_some())
+                || (self.implementer.is_some() && self.part.is_some())
         }
-        if (brand.is_some() && vendor_id.is_some()) || (implementer.is_some() && part.is_some()) {
-            break;
+
+        fn convert(mut self) -> (usize, String, String) {
+            let (vendor_id, brand) = if let (Some(implementer), Some(part)) =
+                (self.implementer.take(), self.part.take())
+            {
+                let vendor_id = get_arm_implementer(implementer).map(String::from);
+                // It's possible to "model name" even with an ARM CPU, so just in case we can't retrieve
+                // the brand from "CPU part", we will then use the value from "model name".
+                //
+                // Example from raspberry pi 3B+:
+                //
+                // ```
+                // model name      : ARMv7 Processor rev 4 (v7l)
+                // CPU implementer : 0x41
+                // CPU part        : 0xd03
+                // ```
+                let brand = get_arm_part(implementer, part)
+                    .map(String::from)
+                    .or_else(|| self.brand.take());
+                (vendor_id, brand)
+            } else {
+                (self.vendor_id.take(), self.brand.take())
+            };
+            (
+                self.index,
+                vendor_id.unwrap_or_default(),
+                brand.unwrap_or_default(),
+            )
         }
     }
-    if let (Some(implementer), Some(part)) = (implementer, part) {
-        vendor_id = get_arm_implementer(implementer).map(String::from);
-        // It's possible to "model name" even with an ARM CPU, so just in case we can't retrieve
-        // the brand from "CPU part", we will then use the value from "model name".
-        //
-        // Example from raspberry pi 3B+:
-        //
-        // ```
-        // model name      : ARMv7 Processor rev 4 (v7l)
-        // CPU implementer : 0x41
-        // CPU part        : 0xd03
-        // ```
-        brand = get_arm_part(implementer, part).map(String::from).or(brand);
+
+    let mut cpus: HashMap<usize, (String, String)> = HashMap::new();
+    let mut lines = s.split('\n');
+    while let Some(line) = lines.next() {
+        if is_new_processor(line) {
+            let index = match line.split(':').nth(1).and_then(|i| i.parse::<usize>().ok()) {
+                Some(index) => index,
+                None => {
+                    sysinfo_debug!("Couldn't get processor ID from {line:?}, ignoring this core");
+                    continue;
+                }
+            };
+
+            let mut info = CpuInfo {
+                index,
+                ..Default::default()
+            };
+
+            #[allow(clippy::while_let_on_iterator)]
+            while let Some(line) = lines.next() {
+                if line.starts_with("vendor_id\t") {
+                    info.vendor_id = Some(get_value(line));
+                } else if line.starts_with("model name\t") {
+                    info.brand = Some(get_value(line));
+                } else if line.starts_with("CPU implementer\t") {
+                    info.implementer = Some(get_hex_value(line));
+                } else if line.starts_with("CPU part\t") {
+                    info.part = Some(get_hex_value(line));
+                } else if info.has_all_info() || is_new_processor(line) {
+                    break;
+                }
+            }
+            let (index, vendor_id, brand) = info.convert();
+            cpus.insert(index, (vendor_id, brand));
+        }
     }
-    (vendor_id.unwrap_or_default(), brand.unwrap_or_default())
+    cpus
 }
diff --git a/src/windows/cpu.rs b/src/windows/cpu.rs
index 3a99886..f1c1dfe 100644
--- a/src/windows/cpu.rs
+++ b/src/windows/cpu.rs
@@ -269,10 +269,7 @@ impl CpusWrapper {
 
     fn init_if_needed(&mut self, refresh_kind: CpuRefreshKind) {
         if self.cpus.is_empty() {
-            let (cpus, vendor_id, brand) = super::tools::init_cpus(refresh_kind);
-            self.cpus = cpus;
-            self.global.vendor_id = vendor_id;
-            self.global.brand = brand;
+            self.cpus = super::tools::init_cpus(refresh_kind);
             self.got_cpu_frequency = refresh_kind.frequency();
         }
     }
diff --git a/src/windows/tools.rs b/src/windows/tools.rs
index a0c3340..27b4197 100644
--- a/src/windows/tools.rs
+++ b/src/windows/tools.rs
@@ -17,7 +17,7 @@ impl KeyHandler {
     }
 }
 
-pub(crate) fn init_cpus(refresh_kind: CpuRefreshKind) -> (Vec<Cpu>, String, String) {
+pub(crate) fn init_cpus(refresh_kind: CpuRefreshKind) -> Vec<Cpu> {
     unsafe {
         let mut sys_info: SYSTEM_INFO = zeroed();
         GetSystemInfo(&mut sys_info);
@@ -37,7 +37,7 @@ pub(crate) fn init_cpus(refresh_kind: CpuRefreshKind) -> (Vec<Cpu>, String, Stri
                 *frequency,
             ));
         }
-        (ret, vendor_id, brand)
+        ret
     }
 }
 


Test patch:
diff --git a/tests/cpu.rs b/tests/cpu.rs
index fbeb65b..edcd575 100644
--- a/tests/cpu.rs
+++ b/tests/cpu.rs
@@ -30,3 +30,17 @@ fn test_physical_core_numbers() {
         assert!(count.unwrap() > 0);
     }
 }
+
+#[test]
+fn test_global_cpu_info_not_set() {
+    use sysinfo::{CpuExt, SystemExt};
+
+    let mut s = sysinfo::System::new();
+    assert_eq!(s.global_cpu_info().vendor_id(), "");
+    assert_eq!(s.global_cpu_info().brand(), "");
+    assert_eq!(s.global_cpu_info().frequency(), 0);
+    s.refresh_cpu();
+    assert_eq!(s.global_cpu_info().vendor_id(), "");
+    assert_eq!(s.global_cpu_info().brand(), "");
+    assert_eq!(s.global_cpu_info().frequency(), 0);
+}