papertigers / rust-kstat

Rust kstat bindings for illumos based systems
MIT License
4 stars 2 forks source link

KSTAT_DATA_CHAR has different length between illumos-kstat and rust-kstat #5

Open japinli opened 4 months ago

japinli commented 4 months ago

Hi,

When I try to use the following code to retrive CPU info, I find there is a bug about the KstatNameData::DataChar(i8):

use kstat::KstatReader;
use kstat::kstat_named::KstatNamedData::DataChar;

fn main() {
    let reader = KstatReader::new(Some("cpu_info"), None, None, None)
        .expect("failed to create kstat reader");
    let stats = reader.read().expect("failed to read kstat");

    for data in &stats {
        let map = &data.data;
        println!("instance: {},\tcore_id: {:?},\tpg_id: {:?},\tpkg_core_id: {:?}:\tstate: {}",
            data.instance, map["core_id"], map["pg_id"], map["pkg_core_id"], map["state"]);
    }
}

Here is the output:

[...]
instance: 61,   core_id: DataInt64(27), pg_id: DataInt64(34),   pkg_core_id: DataInt64(11):     state: DataChar(111)
instance: 62,   core_id: DataInt64(12), pg_id: DataInt64(35),   pkg_core_id: DataInt64(12):     state: DataChar(111)
instance: 63,   core_id: DataInt64(28), pg_id: DataInt64(36),   pkg_core_id: DataInt64(12):     state: DataChar(115)

Note that instance 61 and 62, both of them have state: DataChar(111), however, they are not in the same state (61 on-line, 62 off-line), see:

$ psrinfo -v
Status of virtual processor 61 as of: 06/28/2024 16:37:35
  on-line since 06/27/2024 11:56:04.
  The i386 processor operates at 2300 MHz,
        and has an i387 compatible floating point processor.
Status of virtual processor 62 as of: 06/28/2024 16:37:35
  off-line since 06/28/2024 16:36:43.
  The i386 processor operates at 2300 MHz,
        and has an i387 compatible floating point processor.
Status of virtual processor 63 as of: 06/28/2024 16:37:35
  spare since 06/28/2024 16:01:43.
  The i386 processor operates at 2300 MHz,
        and has an i387 compatible floating point processor.

The instance 64 in the spare state has a different DataChar(115) that begins with s character.

The bug caused by rust-kstat only fetches the first character for KSTAT_DATA_CHAR, see [1] and [2], however, illumos use 16-byte for KSTAT_DATA_CHAR, see [3].

[1] https://github.com/papertigers/rust-kstat/blob/master/src/kstat_named.rs#L6 [2] https://github.com/papertigers/rust-kstat/blob/master/src/ffi.rs#L77 [3] https://github.com/illumos/illumos-gate/blob/master/usr/src/cmd/stat/kstat/kstat.h#L49

japinli commented 4 months ago

Here is a POC patch to fix this bug:

diff --git a/src/ffi.rs b/src/ffi.rs
index ca22a25..0a060d1 100644
--- a/src/ffi.rs
+++ b/src/ffi.rs
@@ -74,8 +74,9 @@ impl kstat_named_t {
         let cstr = unsafe { CStr::from_ptr(self.name.as_ptr()) };
         cstr.to_string_lossy()
     }
-    pub fn value_as_char(&self) -> c_char {
-        c_char::from_le(self.value[0] as i8)
+    pub fn value_as_str(&self) -> String {
+        let cstr = unsafe { CStr::from_ptr(self.value.as_ptr() as *const c_char) };
+        cstr.to_string_lossy().into_owned()
     }

     pub fn value_as_i32(&self) -> i32 {
diff --git a/src/kstat_named.rs b/src/kstat_named.rs
index 96bbebf..bfbdfbf 100644
--- a/src/kstat_named.rs
+++ b/src/kstat_named.rs
@@ -4,8 +4,6 @@ use std::borrow::Cow;
 /// The types of data a kstat named/value pair can contain
 #[derive(Debug)]
 pub enum KstatNamedData {
-    /// KSTAT_DATA_CHAR
-    DataChar(i8),
     /// KSTAT_DATA_INT32
     DataInt32(i32),
     /// KSTAT_DATA_UINT32
@@ -14,7 +12,7 @@ pub enum KstatNamedData {
     DataInt64(i64),
     /// KSTAT_DATA_UINT64 or KSTAT_DATA_ULONG
     DataUInt64(u64),
-    /// KSTAT_DATA_STRING
+    /// KSTAT_DATA_CHAR or KSTAT_DATA_STRING
     DataString(String),
 }

@@ -44,7 +42,9 @@ impl KstatNamed {
 impl<'a> From<&'a KstatNamed> for KstatNamedData {
     fn from(t: &'a KstatNamed) -> Self {
         match t.get_data_type() {
-            ffi::KSTAT_DATA_CHAR => KstatNamedData::DataChar(unsafe { (*t.inner).value_as_char() }),
+            ffi::KSTAT_DATA_CHAR => {
+                KstatNamedData::DataString(unsafe { (*t.inner).value_as_str() })
+            }
             ffi::KSTAT_DATA_INT32 => {
                 KstatNamedData::DataInt32(unsafe { (*t.inner).value_as_i32() })
             }