Browse Source

aya: remove unwrap and NonZero* in info

Addresses the feedback from #1007:
- remove panic from `unwrap` and `expect`
- Option<NonZero*> => Option<int> with `0` mapping to `None`

Refs: #1007
tyrone-wu 6 months ago
parent
commit
02d1db5

+ 4 - 4
aya-log/src/lib.rs

@@ -136,21 +136,21 @@ impl EbpfLogger {
     ) -> Result<EbpfLogger, Error> {
         let program_info = loaded_programs()
             .filter_map(|info| info.ok())
-            .find(|info| info.id().is_some_and(|id| id.get() == program_id))
+            .find(|info| info.id() == program_id)
             .ok_or(Error::ProgramNotFound)?;
 
         let map = program_info
             .map_ids()
             .map_err(Error::ProgramError)?
-            .expect("`map_ids` field in `bpf_prog_info` not available")
+            .ok_or_else(|| Error::MapNotFound)?
             .iter()
-            .filter_map(|id| MapInfo::from_id(id.get()).ok())
+            .filter_map(|id| MapInfo::from_id(*id).ok())
             .find(|map_info| match map_info.name_as_str() {
                 Some(name) => name == MAP_NAME,
                 None => false,
             })
             .ok_or(Error::MapNotFound)?;
-        let map = MapData::from_id(map.id().unwrap().get()).map_err(Error::MapError)?;
+        let map = MapData::from_id(map.id()).map_err(Error::MapError)?;
 
         Self::read_logs_async(Map::PerfEventArray(map), logger)?;
 

+ 13 - 25
aya/src/maps/info.rs

@@ -2,7 +2,6 @@
 
 use std::{
     ffi::CString,
-    num::NonZeroU32,
     os::fd::{AsFd as _, BorrowedFd},
     path::Path,
 };
@@ -19,6 +18,8 @@ use crate::{
 };
 
 /// Provides Provides metadata information about a loaded eBPF map.
+///
+/// Introduced in kernel v4.13.
 #[doc(alias = "bpf_map_info")]
 #[derive(Debug)]
 pub struct MapInfo(pub(crate) bpf_map_info);
@@ -49,38 +50,30 @@ impl MapInfo {
 
     /// The unique ID for this map.
     ///
-    /// `None` is returned if the field is not available.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn id(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.id)
+    pub fn id(&self) -> u32 {
+        self.0.id
     }
 
     /// The key size for this map in bytes.
     ///
-    /// `None` is returned if the field is not available.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn key_size(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.key_size)
+    pub fn key_size(&self) -> u32 {
+        self.0.key_size
     }
 
     /// The value size for this map in bytes.
     ///
-    /// `None` is returned if the field is not available.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn value_size(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.value_size)
+    pub fn value_size(&self) -> u32 {
+        self.0.value_size
     }
 
     /// The maximum number of entries in this map.
     ///
-    /// `None` is returned if the field is not available.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn max_entries(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.max_entries)
+    pub fn max_entries(&self) -> u32 {
+        self.0.max_entries
     }
 
     /// The flags used in loading this map.
@@ -103,14 +96,9 @@ impl MapInfo {
     ///
     /// Introduced in kernel v4.15.
     pub fn name_as_str(&self) -> Option<&str> {
-        let name = std::str::from_utf8(self.name()).ok();
-        if let Some(name_str) = name {
-            // Char in program name was introduced in the same commit as map name
-            if FEATURES.bpf_name() || !name_str.is_empty() {
-                return name;
-            }
-        }
-        None
+        let name = std::str::from_utf8(self.name()).ok()?;
+        // Char in program name was introduced in the same commit as map name
+        (FEATURES.bpf_name() || !name.is_empty()).then_some(name)
     }
 
     /// Returns a file descriptor referencing the map.

+ 4 - 4
aya/src/maps/mod.rs

@@ -1217,11 +1217,11 @@ mod tests {
                 .map(|map_info| {
                     let map_info = map_info.unwrap();
                     (
-                        map_info.id().unwrap().get(),
-                        map_info.key_size().unwrap().get(),
-                        map_info.value_size().unwrap().get(),
+                        map_info.id(),
+                        map_info.key_size(),
+                        map_info.value_size(),
                         map_info.map_flags(),
-                        map_info.max_entries().unwrap().get(),
+                        map_info.max_entries(),
                         map_info.fd().unwrap().as_fd().as_raw_fd(),
                     )
                 })

+ 21 - 49
aya/src/programs/info.rs

@@ -2,7 +2,6 @@
 
 use std::{
     ffi::CString,
-    num::{NonZeroU32, NonZeroU64},
     os::fd::{AsFd as _, BorrowedFd},
     path::Path,
     time::{Duration, SystemTime},
@@ -46,11 +45,9 @@ impl ProgramInfo {
 
     /// The unique ID for this program.
     ///
-    /// `None` is returned if the field is not available.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn id(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.id)
+    pub fn id(&self) -> u32 {
+        self.0.id
     }
 
     /// The program tag.
@@ -59,11 +56,9 @@ impl ProgramInfo {
     /// [`Self::id()`]. A program's ID can vary every time it's loaded or unloaded, but the tag
     /// will remain the same.
     ///
-    /// `None` is returned if the field is not available.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn tag(&self) -> Option<NonZeroU64> {
-        NonZeroU64::new(u64::from_be_bytes(self.0.tag))
+    pub fn tag(&self) -> u64 {
+        u64::from_be_bytes(self.0.tag)
     }
 
     /// The size in bytes of the program's JIT-compiled machine code.
@@ -71,11 +66,9 @@ impl ProgramInfo {
     /// Note that this field is only updated when BPF JIT compiler is enabled. Kernels v4.15 and
     /// above may already have it enabled by default.
     ///
-    /// `None` is returned if the field is not available, or if the JIT compiler is not enabled.
-    ///
     /// Introduced in kernel v4.13.
-    pub fn size_jitted(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.jited_prog_len)
+    pub fn size_jitted(&self) -> u32 {
+        self.0.jited_prog_len
     }
 
     /// The size in bytes of the program's translated eBPF bytecode.
@@ -86,8 +79,8 @@ impl ProgramInfo {
     /// `None` is returned if the field is not available.
     ///
     /// Introduced in kernel v4.15.
-    pub fn size_translated(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.xlated_prog_len)
+    pub fn size_translated(&self) -> Option<u32> {
+        (self.0.xlated_prog_len > 0).then_some(self.0.xlated_prog_len)
     }
 
     /// The time when the program was loaded.
@@ -96,11 +89,7 @@ impl ProgramInfo {
     ///
     /// Introduced in kernel v4.15.
     pub fn loaded_at(&self) -> Option<SystemTime> {
-        if self.0.load_time > 0 {
-            Some(boot_time() + Duration::from_nanos(self.0.load_time))
-        } else {
-            None
-        }
+        (self.0.load_time > 0).then_some(boot_time() + Duration::from_nanos(self.0.load_time))
     }
 
     /// The user ID of the process who loaded the program.
@@ -110,11 +99,7 @@ impl ProgramInfo {
     /// Introduced in kernel v4.15.
     pub fn created_by_uid(&self) -> Option<u32> {
         // This field was introduced in the same commit as `load_time`.
-        if self.0.load_time > 0 {
-            Some(self.0.created_by_uid)
-        } else {
-            None
-        }
+        (self.0.load_time > 0).then_some(self.0.created_by_uid)
     }
 
     /// The IDs of the maps used by the program.
@@ -122,17 +107,11 @@ impl ProgramInfo {
     /// `None` is returned if the field is not available.
     ///
     /// Introduced in kernel v4.15.
-    pub fn map_ids(&self) -> Result<Option<Vec<NonZeroU32>>, ProgramError> {
+    pub fn map_ids(&self) -> Result<Option<Vec<u32>>, ProgramError> {
         if FEATURES.prog_info_map_ids() {
             let mut map_ids = vec![0u32; self.0.nr_map_ids as usize];
             bpf_prog_get_info_by_fd(self.fd()?.as_fd(), &mut map_ids)?;
-
-            Ok(Some(
-                map_ids
-                    .into_iter()
-                    .map(|id| NonZeroU32::new(id).unwrap())
-                    .collect(),
-            ))
+            Ok(Some(map_ids))
         } else {
             Ok(None)
         }
@@ -151,13 +130,8 @@ impl ProgramInfo {
     ///
     /// Introduced in kernel v4.15.
     pub fn name_as_str(&self) -> Option<&str> {
-        let name = std::str::from_utf8(self.name()).ok();
-        if let Some(name_str) = name {
-            if FEATURES.bpf_name() || !name_str.is_empty() {
-                return name;
-            }
-        }
-        None
+        let name = std::str::from_utf8(self.name()).ok()?;
+        (FEATURES.bpf_name() || !name.is_empty()).then_some(name)
     }
 
     /// Returns true if the program is defined with a GPL-compatible license.
@@ -166,18 +140,16 @@ impl ProgramInfo {
     ///
     /// Introduced in kernel v4.18.
     pub fn gpl_compatible(&self) -> Option<bool> {
-        if FEATURES.prog_info_gpl_compatible() {
-            Some(self.0.gpl_compatible() != 0)
-        } else {
-            None
-        }
+        FEATURES
+            .prog_info_gpl_compatible()
+            .then_some(self.0.gpl_compatible() != 0)
     }
 
     /// The BTF ID for the program.
     ///
     /// Introduced in kernel v5.0.
-    pub fn btf_id(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.btf_id)
+    pub fn btf_id(&self) -> Option<u32> {
+        (self.0.btf_id > 0).then_some(self.0.btf_id)
     }
 
     /// The accumulated time that the program has been actively running.
@@ -213,8 +185,8 @@ impl ProgramInfo {
     /// `None` is returned if the field is not available.
     ///
     /// Introduced in kernel v5.16.
-    pub fn verified_instruction_count(&self) -> Option<NonZeroU32> {
-        NonZeroU32::new(self.0.verified_insns)
+    pub fn verified_instruction_count(&self) -> Option<u32> {
+        (self.0.verified_insns > 0).then_some(self.0.verified_insns)
     }
 
     /// How much memory in bytes has been allocated and locked for the program.

+ 33 - 46
test/integration-test/src/tests/info.rs

@@ -1,4 +1,9 @@
 //! Tests the Info API.
+// TODO: Figure out a way to assert that field is truely not present.
+//       We can call `bpf_obj_get_info_by_fd()` and fill our target field with arbitrary data.
+//       `E2BIG` error from `bpf_check_uarg_tail_zero()` will detect if we're accessing fields that
+//       isn't supported on the kernel.
+//       Issue is that `bpf_obj_get_info_by_fd()` will need to be public. :/
 
 use std::{fs, panic, path::Path, time::SystemTime};
 
@@ -77,13 +82,10 @@ fn test_program_info() {
         test_prog.program_type().unwrap_or(ProgramType::Unspecified),
         KernelVersion::new(4, 13, 0),
     );
-    kernel_assert!(test_prog.id().is_some(), KernelVersion::new(4, 13, 0));
-    kernel_assert!(test_prog.tag().is_some(), KernelVersion::new(4, 13, 0));
+    kernel_assert!(test_prog.id() > 0, KernelVersion::new(4, 13, 0));
+    kernel_assert!(test_prog.tag() > 0, KernelVersion::new(4, 13, 0));
     if jit_enabled {
-        kernel_assert!(
-            test_prog.size_jitted().is_some(),
-            KernelVersion::new(4, 13, 0),
-        );
+        kernel_assert!(test_prog.size_jitted() > 0, KernelVersion::new(4, 13, 0));
     }
     kernel_assert!(
         test_prog.size_translated().is_some(),
@@ -93,8 +95,9 @@ fn test_program_info() {
         test_prog.loaded_at().is_some(),
         KernelVersion::new(4, 15, 0),
     );
-    kernel_assert!(
-        test_prog.created_by_uid().is_some_and(|uid| uid == 0),
+    kernel_assert_eq!(
+        Some(0),
+        test_prog.created_by_uid(),
         KernelVersion::new(4, 15, 0),
     );
     let maps = test_prog.map_ids().unwrap();
@@ -102,14 +105,14 @@ fn test_program_info() {
         maps.is_some_and(|ids| ids.is_empty()),
         KernelVersion::new(4, 15, 0),
     );
-    kernel_assert!(
-        test_prog
-            .name_as_str()
-            .is_some_and(|name| name == "simple_prog"),
+    kernel_assert_eq!(
+        Some("simple_prog"),
+        test_prog.name_as_str(),
         KernelVersion::new(4, 15, 0),
     );
-    kernel_assert!(
-        test_prog.gpl_compatible().is_some_and(|gpl| gpl),
+    kernel_assert_eq!(
+        Some(true),
+        test_prog.gpl_compatible(),
         KernelVersion::new(4, 18, 0),
     );
     kernel_assert!(
@@ -255,9 +258,9 @@ fn list_loaded_maps() {
     if let Ok(info) = &prog.info() {
         if let Some(map_ids) = info.map_ids().unwrap() {
             assert_eq!(2, map_ids.len());
-            for id in map_ids.iter() {
+            for id in map_ids {
                 assert!(
-                    maps.iter().any(|m| &m.id().unwrap() == id),
+                    maps.iter().any(|m| m.id() == id),
                     "expected `loaded_maps()` to have `map_ids` from program",
                 );
             }
@@ -293,21 +296,13 @@ fn test_map_info() {
         hash.map_type().unwrap_or(MapType::Unspecified),
         KernelVersion::new(4, 13, 0),
     );
-    kernel_assert!(hash.id().is_some(), KernelVersion::new(4, 13, 0));
-    kernel_assert!(
-        hash.key_size().is_some_and(|size| size.get() == 4),
-        KernelVersion::new(4, 13, 0),
-    );
-    kernel_assert!(
-        hash.value_size().is_some_and(|size| size.get() == 1),
-        KernelVersion::new(4, 13, 0),
-    );
-    kernel_assert!(
-        hash.max_entries().is_some_and(|size| size.get() == 8),
-        KernelVersion::new(4, 13, 0),
-    );
-    kernel_assert!(
-        hash.name_as_str().is_some_and(|name| name == "BAR"),
+    kernel_assert!(hash.id() > 0, KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(4, hash.key_size(), KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(1, hash.value_size(), KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(8, hash.max_entries(), KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(
+        Some("BAR"),
+        hash.name_as_str(),
         KernelVersion::new(4, 15, 0),
     );
 
@@ -321,21 +316,13 @@ fn test_map_info() {
         array.map_type().unwrap_or(MapType::Unspecified),
         KernelVersion::new(4, 13, 0),
     );
-    kernel_assert!(array.id().is_some(), KernelVersion::new(4, 13, 0));
-    kernel_assert!(
-        array.key_size().is_some_and(|size| size.get() == 4),
-        KernelVersion::new(4, 13, 0),
-    );
-    kernel_assert!(
-        array.value_size().is_some_and(|size| size.get() == 4),
-        KernelVersion::new(4, 13, 0),
-    );
-    kernel_assert!(
-        array.max_entries().is_some_and(|size| size.get() == 10),
-        KernelVersion::new(4, 13, 0),
-    );
-    kernel_assert!(
-        array.name_as_str().is_some_and(|name| name == "FOO"),
+    kernel_assert!(array.id() > 0, KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(4, array.key_size(), KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(4, array.value_size(), KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(10, array.max_entries(), KernelVersion::new(4, 13, 0));
+    kernel_assert_eq!(
+        Some("FOO"),
+        array.name_as_str(),
         KernelVersion::new(4, 15, 0),
     );
 

+ 1 - 1
test/integration-test/src/tests/load.rs

@@ -138,7 +138,7 @@ fn poll_loaded_program_id(name: &str) -> impl Iterator<Item = Option<u32>> + '_
             // program in the middle of a `loaded_programs()` call.
             loaded_programs()
                 .filter_map(|prog| prog.ok())
-                .find_map(|prog| (prog.name() == name.as_bytes()).then(|| prog.id().unwrap().get()))
+                .find_map(|prog| (prog.name() == name.as_bytes()).then(|| prog.id()))
         })
 }
 

+ 11 - 11
xtask/public-api/aya.txt

@@ -1875,14 +1875,14 @@ impl aya::maps::MapInfo
 pub fn aya::maps::MapInfo::fd(&self) -> core::result::Result<aya::maps::MapFd, aya::maps::MapError>
 pub fn aya::maps::MapInfo::from_id(id: u32) -> core::result::Result<Self, aya::maps::MapError>
 pub fn aya::maps::MapInfo::from_pin<P: core::convert::AsRef<std::path::Path>>(path: P) -> core::result::Result<Self, aya::maps::MapError>
-pub fn aya::maps::MapInfo::id(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
-pub fn aya::maps::MapInfo::key_size(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
+pub fn aya::maps::MapInfo::id(&self) -> u32
+pub fn aya::maps::MapInfo::key_size(&self) -> u32
 pub fn aya::maps::MapInfo::map_flags(&self) -> u32
 pub fn aya::maps::MapInfo::map_type(&self) -> core::result::Result<aya::maps::MapType, aya::maps::MapError>
-pub fn aya::maps::MapInfo::max_entries(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
+pub fn aya::maps::MapInfo::max_entries(&self) -> u32
 pub fn aya::maps::MapInfo::name(&self) -> &[u8]
 pub fn aya::maps::MapInfo::name_as_str(&self) -> core::option::Option<&str>
-pub fn aya::maps::MapInfo::value_size(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
+pub fn aya::maps::MapInfo::value_size(&self) -> u32
 impl core::fmt::Debug for aya::maps::MapInfo
 pub fn aya::maps::MapInfo::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
 impl core::marker::Freeze for aya::maps::MapInfo
@@ -8141,24 +8141,24 @@ impl<T> core::convert::From<T> for aya::programs::ProgramFd
 pub fn aya::programs::ProgramFd::from(t: T) -> T
 pub struct aya::programs::ProgramInfo(_)
 impl aya::programs::ProgramInfo
-pub fn aya::programs::ProgramInfo::btf_id(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
+pub fn aya::programs::ProgramInfo::btf_id(&self) -> core::option::Option<u32>
 pub fn aya::programs::ProgramInfo::created_by_uid(&self) -> core::option::Option<u32>
 pub fn aya::programs::ProgramInfo::fd(&self) -> core::result::Result<aya::programs::ProgramFd, aya::programs::ProgramError>
 pub fn aya::programs::ProgramInfo::from_pin<P: core::convert::AsRef<std::path::Path>>(path: P) -> core::result::Result<Self, aya::programs::ProgramError>
 pub fn aya::programs::ProgramInfo::gpl_compatible(&self) -> core::option::Option<bool>
-pub fn aya::programs::ProgramInfo::id(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
+pub fn aya::programs::ProgramInfo::id(&self) -> u32
 pub fn aya::programs::ProgramInfo::loaded_at(&self) -> core::option::Option<std::time::SystemTime>
-pub fn aya::programs::ProgramInfo::map_ids(&self) -> core::result::Result<core::option::Option<alloc::vec::Vec<core::num::nonzero::NonZeroU32>>, aya::programs::ProgramError>
+pub fn aya::programs::ProgramInfo::map_ids(&self) -> core::result::Result<core::option::Option<alloc::vec::Vec<u32>>, aya::programs::ProgramError>
 pub fn aya::programs::ProgramInfo::memory_locked(&self) -> core::result::Result<u32, aya::programs::ProgramError>
 pub fn aya::programs::ProgramInfo::name(&self) -> &[u8]
 pub fn aya::programs::ProgramInfo::name_as_str(&self) -> core::option::Option<&str>
 pub fn aya::programs::ProgramInfo::program_type(&self) -> core::result::Result<aya::programs::ProgramType, aya::programs::ProgramError>
 pub fn aya::programs::ProgramInfo::run_count(&self) -> u64
 pub fn aya::programs::ProgramInfo::run_time(&self) -> core::time::Duration
-pub fn aya::programs::ProgramInfo::size_jitted(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
-pub fn aya::programs::ProgramInfo::size_translated(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
-pub fn aya::programs::ProgramInfo::tag(&self) -> core::option::Option<core::num::nonzero::NonZeroU64>
-pub fn aya::programs::ProgramInfo::verified_instruction_count(&self) -> core::option::Option<core::num::nonzero::NonZeroU32>
+pub fn aya::programs::ProgramInfo::size_jitted(&self) -> u32
+pub fn aya::programs::ProgramInfo::size_translated(&self) -> core::option::Option<u32>
+pub fn aya::programs::ProgramInfo::tag(&self) -> u64
+pub fn aya::programs::ProgramInfo::verified_instruction_count(&self) -> core::option::Option<u32>
 impl core::fmt::Debug for aya::programs::ProgramInfo
 pub fn aya::programs::ProgramInfo::fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result
 impl core::marker::Freeze for aya::programs::ProgramInfo