From 02d1db5fc043fb7af90c14d13de6419ec5b9bcb5 Mon Sep 17 00:00:00 2001 From: tyrone-wu Date: Fri, 6 Sep 2024 20:15:36 +0000 Subject: [PATCH] aya: remove unwrap and NonZero* in info Addresses the feedback from #1007: - remove panic from `unwrap` and `expect` - Option => Option with `0` mapping to `None` Refs: #1007 --- aya-log/src/lib.rs | 8 +-- aya/src/maps/info.rs | 38 ++++-------- aya/src/maps/mod.rs | 8 +-- aya/src/programs/info.rs | 70 +++++++--------------- test/integration-test/src/tests/info.rs | 79 +++++++++++-------------- test/integration-test/src/tests/load.rs | 2 +- xtask/public-api/aya.txt | 22 +++---- 7 files changed, 87 insertions(+), 140 deletions(-) diff --git a/aya-log/src/lib.rs b/aya-log/src/lib.rs index 421ce2a0..37ec1b2c 100644 --- a/aya-log/src/lib.rs +++ b/aya-log/src/lib.rs @@ -136,21 +136,21 @@ impl EbpfLogger { ) -> Result { 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)?; diff --git a/aya/src/maps/info.rs b/aya/src/maps/info.rs index 1d15dc6f..2d36c2a1 100644 --- a/aya/src/maps/info.rs +++ b/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::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::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::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::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. diff --git a/aya/src/maps/mod.rs b/aya/src/maps/mod.rs index 752bd9ff..e74289bb 100644 --- a/aya/src/maps/mod.rs +++ b/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(), ) }) diff --git a/aya/src/programs/info.rs b/aya/src/programs/info.rs index dbcfba3f..3bfa89f3 100644 --- a/aya/src/programs/info.rs +++ b/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::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::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::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::new(self.0.xlated_prog_len) + pub fn size_translated(&self) -> Option { + (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 { - 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 { // 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>, ProgramError> { + pub fn map_ids(&self) -> Result>, 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 { - 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::new(self.0.btf_id) + pub fn btf_id(&self) -> Option { + (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::new(self.0.verified_insns) + pub fn verified_instruction_count(&self) -> Option { + (self.0.verified_insns > 0).then_some(self.0.verified_insns) } /// How much memory in bytes has been allocated and locked for the program. diff --git a/test/integration-test/src/tests/info.rs b/test/integration-test/src/tests/info.rs index e90b31b1..7774709c 100644 --- a/test/integration-test/src/tests/info.rs +++ b/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), ); diff --git a/test/integration-test/src/tests/load.rs b/test/integration-test/src/tests/load.rs index 60025092..f9ba4660 100644 --- a/test/integration-test/src/tests/load.rs +++ b/test/integration-test/src/tests/load.rs @@ -138,7 +138,7 @@ fn poll_loaded_program_id(name: &str) -> impl Iterator> + '_ // 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())) }) } diff --git a/xtask/public-api/aya.txt b/xtask/public-api/aya.txt index 0f952944..3422ee2a 100644 --- a/xtask/public-api/aya.txt +++ b/xtask/public-api/aya.txt @@ -1875,14 +1875,14 @@ impl aya::maps::MapInfo pub fn aya::maps::MapInfo::fd(&self) -> core::result::Result pub fn aya::maps::MapInfo::from_id(id: u32) -> core::result::Result pub fn aya::maps::MapInfo::from_pin>(path: P) -> core::result::Result -pub fn aya::maps::MapInfo::id(&self) -> core::option::Option -pub fn aya::maps::MapInfo::key_size(&self) -> core::option::Option +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 -pub fn aya::maps::MapInfo::max_entries(&self) -> core::option::Option +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 +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 core::convert::From 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 +pub fn aya::programs::ProgramInfo::btf_id(&self) -> core::option::Option pub fn aya::programs::ProgramInfo::created_by_uid(&self) -> core::option::Option pub fn aya::programs::ProgramInfo::fd(&self) -> core::result::Result pub fn aya::programs::ProgramInfo::from_pin>(path: P) -> core::result::Result pub fn aya::programs::ProgramInfo::gpl_compatible(&self) -> core::option::Option -pub fn aya::programs::ProgramInfo::id(&self) -> core::option::Option +pub fn aya::programs::ProgramInfo::id(&self) -> u32 pub fn aya::programs::ProgramInfo::loaded_at(&self) -> core::option::Option -pub fn aya::programs::ProgramInfo::map_ids(&self) -> core::result::Result>, aya::programs::ProgramError> +pub fn aya::programs::ProgramInfo::map_ids(&self) -> core::result::Result>, aya::programs::ProgramError> pub fn aya::programs::ProgramInfo::memory_locked(&self) -> core::result::Result 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 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 -pub fn aya::programs::ProgramInfo::size_translated(&self) -> core::option::Option -pub fn aya::programs::ProgramInfo::tag(&self) -> core::option::Option -pub fn aya::programs::ProgramInfo::verified_instruction_count(&self) -> core::option::Option +pub fn aya::programs::ProgramInfo::size_jitted(&self) -> u32 +pub fn aya::programs::ProgramInfo::size_translated(&self) -> core::option::Option +pub fn aya::programs::ProgramInfo::tag(&self) -> u64 +pub fn aya::programs::ProgramInfo::verified_instruction_count(&self) -> core::option::Option 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