tree: untangle modules from event sections

Stop using SectionId for indexing modules (aka. collectors nowadays); there is no 1:1 relationship between the two. Eg. core events are not tied to a module. To to this introduce a ModuleId enum. Hopefully this will go away once we remove the module abstraction. Signed-off-by: Antoine Tenart <[email protected]>
retis-org · Sep 25, 2024 · ec56446 · ec56446
1 parent a46b4a7
commit ec56446
Show file tree

Hide file tree

Showing 4 changed files with 76 additions and 49 deletions.
diff --git a/retis-events/src/events.rs b/retis-events/src/events.rs
@@ -34,7 +34,7 @@
 #![allow(dead_code)] // FIXME
 #![allow(clippy::wrong_self_convention)]
 
-use std::{any::Any, collections::HashMap, fmt, str::FromStr};
+use std::{any::Any, collections::HashMap, fmt};
 
 use anyhow::{anyhow, bail, Result};
 use log::debug;
@@ -195,29 +195,6 @@ pub enum SectionId {
     _MAX = 12,
 }
 
-impl FromStr for SectionId {
-    type Err = anyhow::Error;
-
-    /// Constructs an SectionId from a section unique str identifier.
-    fn from_str(val: &str) -> Result<Self> {
-        use SectionId::*;
-        Ok(match val {
-            "common" => Common,
-            "kernel" => Kernel,
-            "userspace" => Userspace,
-            "tracking" => Tracking,
-            "skb-tracking" => SkbTracking,
-            "skb-drop" => SkbDrop,
-            "skb" => Skb,
-            "ovs" => Ovs,
-            "nft" => Nft,
-            "ct" => Ct,
-            "startup" => Startup,
-            x => bail!("Can't construct a SectionId from {}", x),
-        })
-    }
-}
-
 impl SectionId {
     /// Constructs an SectionId from a section unique identifier
     pub fn from_u8(val: u8) -> Result<SectionId> {

diff --git a/retis/src/collect/collector.rs b/retis/src/collect/collector.rs
@@ -44,9 +44,9 @@ use crate::{
         probe::{kernel::probe_stack::ProbeStack, *},
         tracking::{gc::TrackingGC, skb_tracking::init_tracking},
     },
-    events::{EventResult, SectionId},
+    events::EventResult,
     helpers::{signals::Running, time::*},
-    module::Modules,
+    module::{ModuleId, Modules},
 };
 
 /// Generic trait representing a collector. All collectors are required to
@@ -110,7 +110,7 @@ pub(crate) struct Collectors {
     tracking_gc: Option<TrackingGC>,
     // Keep a reference on the tracking configuration map.
     tracking_config_map: Option<libbpf_rs::MapHandle>,
-    loaded: Vec<SectionId>,
+    loaded: Vec<ModuleId>,
     // Retis events factory.
     events_factory: Arc<RetisEventsFactory>,
 }
@@ -222,7 +222,7 @@ impl Collectors {
 
         // Try initializing all collectors.
         for name in &collect.args()?.collectors {
-            let id = SectionId::from_str(name)?;
+            let id = ModuleId::from_str(name)?;
             let c = self
                 .modules
                 .get_collector(&id)
@@ -642,10 +642,10 @@ mod tests {
     fn register_collectors() -> Result<()> {
         let mut group = Modules::new()?;
         assert!(group
-            .register(SectionId::Skb, Box::new(DummyCollectorA::new()?),)
+            .register(ModuleId::Skb, Box::new(DummyCollectorA::new()?),)
             .is_ok());
         assert!(group
-            .register(SectionId::Ovs, Box::new(DummyCollectorB::new()?),)
+            .register(ModuleId::Ovs, Box::new(DummyCollectorB::new()?),)
             .is_ok());
         Ok(())
     }
@@ -654,10 +654,10 @@ mod tests {
     fn register_uniqueness() -> Result<()> {
         let mut group = Modules::new()?;
         assert!(group
-            .register(SectionId::Skb, Box::new(DummyCollectorA::new()?),)
+            .register(ModuleId::Skb, Box::new(DummyCollectorA::new()?),)
             .is_ok());
         assert!(group
-            .register(SectionId::Skb, Box::new(DummyCollectorA::new()?),)
+            .register(ModuleId::Skb, Box::new(DummyCollectorA::new()?),)
             .is_err());
         Ok(())
     }
@@ -668,8 +668,8 @@ mod tests {
         let mut dummy_a = Box::new(DummyCollectorA::new()?);
         let mut dummy_b = Box::new(DummyCollectorB::new()?);
 
-        group.register(SectionId::Skb, Box::new(DummyCollectorA::new()?))?;
-        group.register(SectionId::Ovs, Box::new(DummyCollectorB::new()?))?;
+        group.register(ModuleId::Skb, Box::new(DummyCollectorA::new()?))?;
+        group.register(ModuleId::Ovs, Box::new(DummyCollectorB::new()?))?;
 
         let mut collectors = Collectors::new(group)?;
         let mut mgr = ProbeBuilderManager::new()?;
@@ -693,8 +693,8 @@ mod tests {
         let mut dummy_a = Box::new(DummyCollectorA::new()?);
         let mut dummy_b = Box::new(DummyCollectorB::new()?);
 
-        group.register(SectionId::Skb, Box::new(DummyCollectorA::new()?))?;
-        group.register(SectionId::Ovs, Box::new(DummyCollectorB::new()?))?;
+        group.register(ModuleId::Skb, Box::new(DummyCollectorA::new()?))?;
+        group.register(ModuleId::Ovs, Box::new(DummyCollectorB::new()?))?;
 
         let mut collectors = Collectors::new(group)?;
 

diff --git a/retis/src/module/module.rs b/retis/src/module/module.rs
@@ -1,4 +1,4 @@
-use std::collections::HashMap;
+use std::{collections::HashMap, fmt, str::FromStr};
 
 use anyhow::{bail, Result};
 
@@ -12,9 +12,59 @@ use crate::{
         events::{CommonEventFactory, EventSectionFactory, FactoryId, SectionFactories},
         probe::{kernel::KernelEventFactory, user::UserEventFactory},
     },
-    events::*,
 };
 
+/// Module identifiers.
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+pub(crate) enum ModuleId {
+    SkbTracking,
+    Skb,
+    SkbDrop,
+    Ovs,
+    Nft,
+    Ct,
+}
+
+impl ModuleId {
+    /// Converts a ModuleId to a section unique str identifier.
+    pub fn to_str(self) -> &'static str {
+        use ModuleId::*;
+        match self {
+            SkbTracking => "skb-tracking",
+            SkbDrop => "skb-drop",
+            Skb => "skb",
+            Ovs => "ovs",
+            Nft => "nft",
+            Ct => "ct",
+        }
+    }
+}
+
+impl FromStr for ModuleId {
+    type Err = anyhow::Error;
+
+    /// Constructs a ModuleId from a section unique str identifier.
+    fn from_str(val: &str) -> Result<Self> {
+        use ModuleId::*;
+        Ok(match val {
+            "skb-tracking" => SkbTracking,
+            "skb-drop" => SkbDrop,
+            "skb" => Skb,
+            "ovs" => Ovs,
+            "nft" => Nft,
+            "ct" => Ct,
+            x => bail!("Can't construct a ModuleId from {}", x),
+        })
+    }
+}
+
+// Allow using ModuleId in log messages.
+impl fmt::Display for ModuleId {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{self:?}")
+    }
+}
+
 /// Trait that must be implemented by Modules
 pub(crate) trait Module {
     /// Return a Collector used for collect command
@@ -29,7 +79,7 @@ pub(crate) trait Module {
 /// to manipulate them.
 pub(crate) struct Modules {
     /// Set of registered modules we can use.
-    modules: HashMap<SectionId, Box<dyn Module>>,
+    modules: HashMap<ModuleId, Box<dyn Module>>,
 }
 
 impl Modules {
@@ -50,7 +100,7 @@ impl Modules {
     ///         Box::new(SecondModule::new()?,
     ///     )?;
     /// ```
-    pub(crate) fn register(&mut self, id: SectionId, module: Box<dyn Module>) -> Result<&mut Self> {
+    pub(crate) fn register(&mut self, id: ModuleId, module: Box<dyn Module>) -> Result<&mut Self> {
         // Ensure uniqueness of the module name. This is important as their
         // name is used as a key.
         if self.modules.contains_key(&id) {
@@ -63,15 +113,15 @@ impl Modules {
 
     /// Get an hashmap of all the collectors available in the registered
     /// modules.
-    pub(crate) fn collectors(&mut self) -> HashMap<&SectionId, &mut dyn Collector> {
+    pub(crate) fn collectors(&mut self) -> HashMap<&ModuleId, &mut dyn Collector> {
         self.modules
             .iter_mut()
             .map(|(id, m)| (id, m.collector()))
             .collect()
     }
 
     /// Get a specific collector, if found in the registered modules.
-    pub(crate) fn get_collector(&mut self, id: &SectionId) -> Option<&mut dyn Collector> {
+    pub(crate) fn get_collector(&mut self, id: &ModuleId) -> Option<&mut dyn Collector> {
         self.modules.get_mut(id).map(|m| m.collector())
     }
 
@@ -98,12 +148,12 @@ pub(crate) fn get_modules() -> Result<Modules> {
 
     // Register all collectors here.
     group
-        .register(SectionId::SkbTracking, Box::new(SkbTrackingModule::new()?))?
-        .register(SectionId::Skb, Box::new(SkbModule::new()?))?
-        .register(SectionId::SkbDrop, Box::new(SkbDropModule::new()?))?
-        .register(SectionId::Ovs, Box::new(OvsModule::new()?))?
-        .register(SectionId::Nft, Box::new(NftModule::new()?))?
-        .register(SectionId::Ct, Box::new(CtModule::new()?))?;
+        .register(ModuleId::SkbTracking, Box::new(SkbTrackingModule::new()?))?
+        .register(ModuleId::Skb, Box::new(SkbModule::new()?))?
+        .register(ModuleId::SkbDrop, Box::new(SkbDropModule::new()?))?
+        .register(ModuleId::Ovs, Box::new(OvsModule::new()?))?
+        .register(ModuleId::Nft, Box::new(NftModule::new()?))?
+        .register(ModuleId::Ct, Box::new(CtModule::new()?))?;
 
     Ok(group)
 }

diff --git a/retis/src/module/skb/bpf.rs b/retis/src/module/skb/bpf.rs
@@ -17,7 +17,7 @@ use crate::{
     core::events::{
         parse_raw_section, BpfRawSection, EventSectionFactory, FactoryId, RawEventSectionFactory,
     },
-    event_byte_array,
+    event_byte_array, event_section_factory,
     events::{net::RawPacket, *},
     helpers, raw_event_section,
 };