Add CHANGELOG entry for #269

This change addresses a few remaining issues of #269 and adds a CHANGELOG entry. Signed-off-by: Daniel Müller <[email protected]>
libbpf · Aug 4, 2023 · e483371 · e483371
1 parent d9409ea
commit e483371
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 43 deletions.
diff --git a/examples/bpf_query/src/main.rs b/examples/bpf_query/src/main.rs
@@ -28,7 +28,10 @@ fn prog(args: ProgArgs) {
     for prog in query::ProgInfoIter::with_query_opts(opts) {
         println!(
             "name={:<16} type={:<15} run_count={:<2} runtime_ns={}",
-            prog.name, prog.ty, prog.run_cnt, prog.run_time_ns
+            prog.name.to_string_lossy(),
+            prog.ty,
+            prog.run_cnt,
+            prog.run_time_ns
         );
         if args.disassemble {
             #[cfg(target_arch = "x86_64")]
@@ -56,13 +59,18 @@ fn prog(args: ProgArgs) {
 
 fn map() {
     for map in query::MapInfoIter::default() {
-        println!("name={:<16} type={}", map.name, map.ty);
+        println!("name={:<16} type={}", map.name.to_string_lossy(), map.ty);
     }
 }
 
 fn btf() {
     for btf in query::BtfInfoIter::default() {
-        println!("id={:4} name={} size={}", btf.id, btf.name, btf.btf.len());
+        println!(
+            "id={:4} name={} size={}",
+            btf.id,
+            btf.name.to_string_lossy(),
+            btf.btf.len()
+        );
     }
 }
 

diff --git a/libbpf-rs/CHANGELOG.md b/libbpf-rs/CHANGELOG.md
@@ -1,3 +1,9 @@
+Unreleased
+----------
+- Overhauled `query::ProgramInfo` and `query::ProgInfoIter` to make them more
+  readily usable
+
+
 0.21.2
 ------
 - Enabled key iteration on `MapHandle` objects (formerly possible only on `Map`

diff --git a/libbpf-rs/src/query.rs b/libbpf-rs/src/query.rs
@@ -6,15 +6,15 @@
 //!
 //! let mut iter = ProgInfoIter::default();
 //! for prog in iter {
-//!     println!("{}", prog.name);
+//!     println!("{}", prog.name.to_string_lossy());
 //! }
 //! ```
 
-use core::ffi::c_void;
 use std::convert::TryFrom;
+use std::ffi::c_void;
+use std::ffi::CString;
 use std::mem::size_of_val;
 use std::os::raw::c_char;
-use std::string::String;
 use std::time::Duration;
 
 use nix::errno;
@@ -91,19 +91,6 @@ macro_rules! gen_info_impl {
     };
 }
 
-fn name_arr_to_string(a: &[c_char], default: &str) -> String {
-    let converted_arr: Vec<u8> = a
-        .iter()
-        .take_while(|x| **x != 0)
-        .map(|x| *x as u8)
-        .collect();
-    if !converted_arr.is_empty() {
-        String::from_utf8(converted_arr).unwrap_or_else(|_| default.to_string())
-    } else {
-        default.to_string()
-    }
-}
-
 /// BTF Line information
 #[derive(Clone, Debug)]
 pub struct LineInfo {
@@ -131,17 +118,17 @@ impl From<&libbpf_sys::bpf_line_info> for LineInfo {
     }
 }
 
+/// Bpf identifier tag
 #[derive(Debug, Clone, Default)]
 #[repr(C)]
-/// Bpf identifier tag
 pub struct Tag([u8; 8]);
 
 /// Information about a BPF program
 #[derive(Debug, Clone)]
 // TODO: Document members.
 #[allow(missing_docs)]
 pub struct ProgramInfo {
-    pub name: String,
+    pub name: CString,
     pub ty: ProgramType,
     pub tag: Tag,
     pub id: u32,
@@ -169,15 +156,15 @@ pub struct ProgramInfo {
     pub run_cnt: u64,
 }
 
-#[derive(Default, Debug)]
 /// An iterator for the information of loaded bpf programs
+#[derive(Default, Debug)]
 pub struct ProgInfoIter {
     cur_id: u32,
     opts: ProgInfoQueryOptions,
 }
 
-#[derive(Clone, Default, Debug)]
 /// Options to query the program info currently loaded
+#[derive(Clone, Default, Debug)]
 pub struct ProgInfoQueryOptions {
     /// Include the vector of bpf instructions in the result
     include_xlated_prog_insns: bool,
@@ -264,17 +251,19 @@ impl ProgInfoQueryOptions {
         self
     }
 
-    /// Include all
+    /// Include everything there is in the query results
     pub fn include_all(self) -> Self {
-        self.include_xlated_prog_insns(true)
-            .include_jited_prog_insns(true)
-            .include_map_ids(true)
-            .include_line_info(true)
-            .include_func_info(true)
-            .include_jited_line_info(true)
-            .include_jited_func_lens(true)
-            .include_prog_tags(true)
-            .include_jited_ksyms(true)
+        Self {
+            include_xlated_prog_insns: true,
+            include_jited_prog_insns: true,
+            include_map_ids: true,
+            include_line_info: true,
+            include_func_info: true,
+            include_jited_line_info: true,
+            include_jited_func_lens: true,
+            include_prog_tags: true,
+            include_jited_ksyms: true,
+        }
     }
 }
 
@@ -299,7 +288,8 @@ impl ProgramInfo {
             unsafe { libbpf_sys::bpf_obj_get_info_by_fd(fd, item_ptr as *mut c_void, &mut len) };
         util::parse_ret(ret)?;
 
-        let name = name_arr_to_string(&item.name, "(?)");
+        // SANITY: `libbpf` should guarantee NUL termination.
+        let name = util::c_char_slice_to_cstr(&item.name).unwrap();
         let ty = match ProgramType::try_from(item.type_) {
             Ok(ty) => ty,
             Err(_) => ProgramType::Unknown,
@@ -379,7 +369,7 @@ impl ProgramInfo {
         util::parse_ret(ret)?;
 
         return Ok(ProgramInfo {
-            name,
+            name: name.to_owned(),
             ty,
             tag: Tag(item.tag),
             id: item.id,
@@ -430,7 +420,8 @@ impl Iterator for ProgInfoIter {
 
             match prog {
                 Ok(p) => return Some(p),
-                Err(e) => eprintln!("Failed to load program: {}", e),
+                // TODO: We should consider bubbling up errors properly.
+                Err(_err) => (),
             }
         }
     }
@@ -441,7 +432,7 @@ impl Iterator for ProgInfoIter {
 // TODO: Document members.
 #[allow(missing_docs)]
 pub struct MapInfo {
-    pub name: String,
+    pub name: CString,
     pub ty: MapType,
     pub id: u32,
     pub key_size: u32,
@@ -459,14 +450,15 @@ pub struct MapInfo {
 
 impl MapInfo {
     fn from_uapi(_fd: i32, s: libbpf_sys::bpf_map_info) -> Option<Self> {
-        let name = name_arr_to_string(&s.name, "(?)");
+        // SANITY: `libbpf` should guarantee NUL termination.
+        let name = util::c_char_slice_to_cstr(&s.name).unwrap();
         let ty = match MapType::try_from(s.type_) {
             Ok(ty) => ty,
             Err(_) => MapType::Unknown,
         };
 
         Some(Self {
-            name,
+            name: name.to_owned(),
             ty,
             id: s.id,
             key_size: s.key_size,
@@ -495,10 +487,9 @@ gen_info_impl!(
 
 /// Information about BPF type format
 #[derive(Debug, Clone)]
-#[allow(missing_docs)]
 pub struct BtfInfo {
     /// The name associated with this btf information in the kernel
-    pub name: String,
+    pub name: CString,
     /// The raw btf bytes from the kernel
     pub btf: Vec<u8>,
     /// The btf id associated with this btf information in the kernel
@@ -532,7 +523,10 @@ impl BtfInfo {
         util::parse_ret(ret)?;
 
         Ok(BtfInfo {
-            name: String::from_utf8(name).unwrap_or_else(|_| "(?)".to_string()),
+            // SANITY: Our buffer contained space for a NUL byte and we set its
+            //         contents to 0. Barring a `libbpf` bug a NUL byte will be
+            //         present.
+            name: CString::from_vec_with_nul(name).unwrap(),
             btf,
             id: item.id,
         })
@@ -568,7 +562,8 @@ impl Iterator for BtfInfoIter {
 
             match info {
                 Ok(i) => return Some(i),
-                Err(e) => eprintln!("Failed to load btf information: {}", e),
+                // TODO: We should consider bubbling up errors properly.
+                Err(_err) => (),
             }
         }
     }

diff --git a/libbpf-rs/src/util.rs b/libbpf-rs/src/util.rs
@@ -1,5 +1,6 @@
 use std::ffi::CStr;
 use std::ffi::CString;
+use std::mem::transmute;
 use std::os::raw::c_char;
 use std::path::Path;
 use std::ptr::NonNull;
@@ -32,6 +33,22 @@ pub fn c_ptr_to_string(p: *const c_char) -> Result<String> {
         .to_owned())
 }
 
+/// Convert a `[c_char]` into a `CStr`.
+pub fn c_char_slice_to_cstr(s: &[c_char]) -> Option<&CStr> {
+    // TODO: Switch to using `CStr::from_bytes_until_nul` once we require
+    //       Rust 1.69.0.
+    let nul_idx = s
+        .iter()
+        .enumerate()
+        .find_map(|(idx, b)| (*b == 0).then_some(idx))?;
+    let cstr =
+        // SAFETY: `c_char` and `u8` are both just one byte plain old data
+        //         types.
+        CStr::from_bytes_with_nul(unsafe { transmute::<&[c_char], &[u8]>(&s[0..=nul_idx]) })
+            .unwrap();
+    Some(cstr)
+}
+
 /// Round up a number to the next multiple of `r`
 pub fn roundup(num: usize, r: usize) -> usize {
     ((num + (r - 1)) / r) * r
@@ -129,4 +146,27 @@ mod tests {
         let num = num_possible_cpus().unwrap();
         assert!(num > 0);
     }
+
+    /// Check that we can convert a `[c_char]` into a `CStr`.
+    #[test]
+    fn c_char_slice_conversion() {
+        let slice = [];
+        assert_eq!(c_char_slice_to_cstr(&slice), None);
+
+        let slice = [0];
+        assert_eq!(
+            c_char_slice_to_cstr(&slice).unwrap(),
+            CStr::from_bytes_with_nul(b"\0").unwrap()
+        );
+
+        let slice = ['a' as _, 'b' as _, 'c' as _, 0 as _];
+        assert_eq!(
+            c_char_slice_to_cstr(&slice).unwrap(),
+            CStr::from_bytes_with_nul(b"abc\0").unwrap()
+        );
+
+        // Missing terminating NUL byte.
+        let slice = ['a' as _, 'b' as _, 'c' as _];
+        assert_eq!(c_char_slice_to_cstr(&slice), None);
+    }
 }