SQUASH: respond to tamir's review

Signed-off-by: astoycos <astoycos@redhat.com>

aya/src/bpf.rs

@@ -854,7 +854,7 @@ impl Bpf {
         self.maps.iter().map(|(name, map)| (name.as_str(), map))
     }
 
-    /// An iterator mutably referencing all the maps.
+    /// An iterator over all the maps that allows modification.
     ///
     /// # Examples
     /// ```no_run

aya/src/maps/mod.rs

@@ -38,6 +38,7 @@
 //! implement the [Pod] trait.
 use std::{
     borrow::BorrowMut,
+    collections::HashSet,
     ffi::CString,
     fmt, io,
     marker::PhantomData,
@@ -175,6 +176,10 @@ pub enum MapError {
         /// The map type
         map_type: u32,
     },
+
+    /// Unable to parse map name from kernel info
+    #[error("Unable to parse map name")]
+    ParseNameError,
 }
 
 /// A map file descriptor.
@@ -309,7 +314,7 @@ impl Map {
     }
 
     /// Removes the pinned map from a BPF filesystem.
-    pub fn unpin(&mut self) -> Result<(), io::Error> {
+    pub fn unpin(&mut self) -> Result<(), PinError> {
         match self {
             Self::Array(map) => map.unpin(),
             Self::PerCpuArray(map) => map.unpin(),
@@ -357,7 +362,7 @@ macro_rules! impl_map_pin {
                     }
 
                     /// Removes the pinned map from a BPF filesystem.
-                    pub fn unpin(mut self) -> Result<(), io::Error> {
+                    pub fn unpin(mut self) -> Result<(), PinError> {
                         let data = self.inner.borrow_mut();
                         data.unpin()
                     }
@@ -504,7 +509,7 @@ pub struct MapData {
     pub(crate) obj: obj::Map,
     pub(crate) fd: RawFd,
     pub(crate) name: String,
-    pub(crate) paths: Vec<PathBuf>,
+    pub(crate) paths: HashSet<PathBuf>,
 }
 
 impl MapData {
@@ -539,7 +544,7 @@ impl MapData {
             obj,
             fd,
             name: name.to_string(),
-            paths: Vec::new(),
+            paths: HashSet::new(),
         })
     }
 
@@ -570,7 +575,7 @@ impl MapData {
                 obj,
                 fd: fd.into_raw_fd(),
                 name: name.to_string(),
-                paths: vec![path],
+                paths: HashSet::from([path]),
             }),
             Err(_) => {
                 let mut map = Self::create(obj, name, btf_fd)?;
@@ -610,9 +615,9 @@ impl MapData {
             obj: parse_map_info(info, PinningType::ByName),
             fd: fd.into_raw_fd(),
             name: std::str::from_utf8(name_bytes)
-                .expect("unable to parse map name from bpf_map_info")
+                .map_err(|_| MapError::ParseNameError)?
                 .to_string(),
-            paths: vec![path.to_path_buf()],
+            paths: HashSet::from([path.to_path_buf()]),
         })
     }
 
@@ -629,9 +634,9 @@ impl MapData {
             obj: parse_map_info(info, PinningType::None),
             fd: fd.into_raw_fd(),
             name: std::str::from_utf8(name_bytes)
-                .expect("unable to parse map name from bpf_map_info")
+                .map_err(|_| MapError::ParseNameError)?
                 .to_string(),
-            paths: Vec::new(),
+            paths: HashSet::new(),
         })
     }
 
@@ -655,25 +660,42 @@ impl MapData {
                 error,
             }
         })?;
-        debug!("Attempting to pin map {name} at {path_string:?}");
+        debug!(
+            "Attempting to pin map {name} at {}",
+            path.as_ref().display()
+        );
         bpf_pin_object(*fd, &path_string).map_err(|(_, io_error)| SyscallError {
             call: "BPF_OBJ_PIN",
             io_error,
         })?;
-        paths.push(path.as_ref().to_path_buf());
+        paths.insert(path.as_ref().to_path_buf());
         Ok(())
     }
 
-    pub(crate) fn unpin(&mut self) -> Result<(), io::Error> {
+    pub(crate) fn unpin(&mut self) -> Result<(), PinError> {
         let Self {
             fd: _,
             paths,
             obj: _,
-            name: _,
+            name,
         } = self;
-        for path in paths.drain(..) {
-            std::fs::remove_file(path)?;
+
+        let mut errors: Vec<(PathBuf, io::Error)> = Vec::new();
+        for path in paths.drain() {
+            std::fs::remove_file(&path).unwrap_or_else(|e| {
+                errors.push((path, e));
+            });
         }
+
+        if !errors.is_empty() {
+            let (paths, errors) = errors.into_iter().unzip();
+            return Err(PinError::UnpinError {
+                name: name.to_string(),
+                paths,
+                errors,
+            });
+        };
+
         Ok(())
     }
 

aya/src/pin.rs

@@ -31,4 +31,17 @@ pub enum PinError {
     /// An error ocurred making a syscall.
     #[error(transparent)]
     SyscallError(#[from] SyscallError),
+
+    /// An error occured unpinning an object.
+    #[error("Failed to remove pins {paths:?} for object {name}")]
+    UnpinError {
+        /// Object name.
+        name: String,
+
+        /// The paths.
+        paths: Vec<std::path::PathBuf>,
+
+        /// The source error
+        errors: Vec<std::io::Error>,
+    },
 }

aya/src/programs/mod.rs

@@ -786,9 +786,13 @@ macro_rules! impl_program_pin{
                 }
 
                 /// Removes the pinned link from the filesystem.
-                pub fn unpin(mut self) -> Result<(), io::Error> {
+                pub fn unpin(mut self) -> Result<(), PinError> {
                     if let Some(path) = self.data.path.take() {
-                        std::fs::remove_file(path)?;
+                        std::fs::remove_file(&path).map_err(|e| PinError::UnpinError{
+                            name: self.data.name.clone().unwrap(),
+                            paths: vec![path],
+                            errors: vec![e]
+                        })?;
                     }
                     Ok(())
                 }

aya/src/util.rs

@@ -338,11 +338,8 @@ pub(crate) fn bytes_of_slice<T: Pod>(val: &[T]) -> &[u8] {
 }
 
 pub(crate) fn bytes_of_c_char(val: &[std::os::raw::c_char]) -> &[u8] {
-    let length = val
-        .iter()
-        .rposition(|ch| *ch != 0)
-        .map(|pos| pos + 1)
-        .unwrap_or(0);
+    // length is the end of the C-style string (nul byte).
+    let length = val.iter().position(|ch| *ch == 0).unwrap_or(0);
 
     // The name field is defined as [std::os::raw::c_char; 16]. c_char may be signed or
     // unsigned depending on the platform; that's why we're using from_raw_parts here

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

@@ -91,11 +91,11 @@ fn pin_lifecycle_multiple_btf_maps() {
     let mut map_pin_by_name: Array<_, u64> =
         bpf.take_map("map_pin_by_name").unwrap().try_into().unwrap();
 
-    let prog: &mut TracePoint = bpf.program_mut("bpf_prog").unwrap().try_into().unwrap();
+    let prog: &mut UProbe = bpf.program_mut("bpf_prog").unwrap().try_into().unwrap();
     prog.load().unwrap();
-    prog.attach("sched", "sched_switch").unwrap();
-
-    thread::sleep(time::Duration::from_secs(3));
+    prog.attach(Some("trigger_bpf_program"), 0, "/proc/self/exe", None)
+        .unwrap();
+    trigger_bpf_program();
 
     let key = 0;
     let val_1 = map_1.get(&key, 0).unwrap();

test/integration-test/src/tests/rbpf.rs

@@ -44,7 +44,7 @@ fn use_map_with_rbpf() {
     );
 
     // Initialize maps:
-    // - fd: 0xCAFE00 or 0xCAFE01 (the 0xCAFE00 part is used to distinguish fds from indices),
+    // - fd: Bitwise OR of the map_id with 0xCAFE00 (used to distinguish fds from indices),
     // - Note that rbpf does not convert fds into real pointers,
     //   so we keeps the pointers to our maps in MULTIMAP_MAPS, to be used in helpers.
     let mut maps = HashMap::new();
@@ -105,13 +105,10 @@ fn use_map_with_rbpf() {
         .expect("Helper failed");
     assert_eq!(vm.execute_program().unwrap(), 0);
 
-    assert_eq!(map_instances[0][0], 24);
-    assert_eq!(map_instances[1][0], 42);
-    assert_eq!(map_instances[2][0], 44);
+    assert_eq!(map_instances, vec![vec![24], vec![42], vec![44]]);
 
     unsafe {
-        MULTIMAP_MAPS[0] = null_mut();
-        MULTIMAP_MAPS[1] = null_mut();
+        MULTIMAP_MAPS.iter_mut().for_each(|v| *v = null_mut());
     }
 }