Refactoring, newtype for DLS Link IDs

Author: FelixMcFelix

xde/src/dls/mod.rs

@@ -8,56 +8,103 @@
 
 pub mod sys;
 
-use crate::mac::mac_client_promisc_type_t;
-use crate::mac::mac_promisc_add;
-use crate::mac::mac_rx_fn;
+use crate::mac::mac_client_handle;
+use crate::mac::MacClient;
 use crate::mac::MacPerimeterHandle;
-use crate::mac::MacPromiscHandle;
 use crate::mac::MacTxFlags;
 use crate::mac::MAC_DROP_ON_NO_DESC;
 use alloc::sync::Arc;
-use core::ffi::c_void;
+use core::ffi::CStr;
+use core::fmt::Display;
 use core::mem::MaybeUninit;
 use core::ptr;
 use illumos_sys_hdrs::c_int;
 use illumos_sys_hdrs::datalink_id_t;
 use illumos_sys_hdrs::uintptr_t;
+use illumos_sys_hdrs::ENOENT;
 use opte::engine::packet::Packet;
 use opte::engine::packet::PacketState;
 pub use sys::*;
 
+/// An integer ID used by DLS to refer to a given link.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub struct LinkId(u32);
+
+impl LinkId {
+    /// Request the link ID for a device using its name.
+    pub fn from_name(name: impl AsRef<CStr>) -> Result<Self, LinkError> {
+        let mut link_id = 0;
+
+        unsafe {
+            match dls_mgmt_get_linkid(name.as_ref().as_ptr(), &mut link_id) {
+                0 => Ok(LinkId(link_id)),
+                ENOENT => Err(LinkError::NotFound),
+                err => Err(LinkError::Other(err)),
+            }
+        }
+    }
+}
+
+impl From<LinkId> for datalink_id_t {
+    fn from(val: LinkId) -> Self {
+        val.0
+    }
+}
+
+/// Errors encountered while querying DLS for a `LinkId`.
+pub enum LinkError {
+    NotFound,
+    Other(i32),
+}
+
+impl Display for LinkError {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        match self {
+            LinkError::NotFound => write!(f, "link not found"),
+            LinkError::Other(e) => write!(f, "unknown error ({e})"),
+        }
+    }
+}
+
+/// A hold on an existing link managed by DLS.
 #[derive(Debug)]
 pub struct DlsLink {
     inner: Option<DlsLinkInner>,
+    link: LinkId,
 }
 
 #[derive(Debug)]
-pub struct DlsLinkInner {
+struct DlsLinkInner {
     dlp: *mut dls_link,
     dlh: dls_dl_handle,
-    link: datalink_id_t,
 }
 
 impl DlsLink {
+    /// Place a hold on an existing link,
     pub fn hold(mph: &MacPerimeterHandle) -> Result<Self, c_int> {
         let mut dlp = ptr::null_mut();
         let mut dlh = ptr::null_mut();
-        let link = mph.linkid();
+        let link = mph.link_id();
 
-        let res = unsafe { dls_devnet_hold_link(link, &mut dlh, &mut dlp) };
+        let res =
+            unsafe { dls_devnet_hold_link(link.into(), &mut dlh, &mut dlp) };
         if res == 0 {
-            Ok(Self { inner: Some(DlsLinkInner { dlp, dlh, link }) })
+            Ok(Self { inner: Some(DlsLinkInner { dlp, dlh }), link })
         } else {
             Err(res)
         }
     }
 
+    pub fn link_id(&self) -> LinkId {
+        self.link
+    }
+
     // XXX: cleanup REQUIRES that we hold the MAC perimeter handle.
     pub fn release(mut self, mph: &MacPerimeterHandle) {
         if let Some(inner) = self.inner.take() {
-            if mph.linkid() != inner.link {
-                panic!("Tried to free link hold with the wrong MAC perimeter: saw {}, wanted {}",
-                    mph.linkid(), inner.link);
+            if mph.link_id() != self.link {
+                panic!("Tried to free link hold with the wrong MAC perimeter: saw {:?}, wanted {:?}",
+                    mph.link_id(),self.link);
             }
             unsafe {
                 dls_devnet_rele_link(inner.dlh, inner.dlp);
@@ -73,9 +120,9 @@ impl DlsLink {
             return Err(-1);
         };
 
-        if mph.linkid() != inner.link {
-            panic!("Tried to open stream with the wrong MAC perimeter: saw {}, wanted {}",
-                mph.linkid(), inner.link);
+        if mph.link_id() != self.link {
+            panic!("Tried to open stream with the wrong MAC perimeter: saw {:?}, wanted {:?}",
+                mph.link_id(),self.link);
         }
 
         let mut stream = MaybeUninit::zeroed();
@@ -86,7 +133,8 @@ impl DlsLink {
             _ = self.inner.take();
             let dld_str = unsafe { stream.assume_init() };
             Ok(DldStream {
-                inner: Some(DldStreamInner { dld_str, link: mph.linkid() }),
+                inner: Some(DldStreamInner { dld_str }),
+                link: mph.link_id(),
             }
             .into())
         } else {
@@ -98,58 +146,30 @@ impl DlsLink {
 
 impl Drop for DlsLink {
     fn drop(&mut self) {
-        if let Some(inner) = self.inner.take() {
+        if self.inner.take().is_some() {
             opte::engine::err!(
-                "dropped hold on link {} without releasing!!",
-                inner.link
+                "dropped hold on link {:?} without releasing!!",
+                self.link
             );
         }
     }
 }
 
+/// A DLS message stream derived from a
 #[derive(Debug)]
 pub struct DldStream {
     inner: Option<DldStreamInner>,
+    link: LinkId,
 }
 
 #[derive(Debug)]
-pub struct DldStreamInner {
+struct DldStreamInner {
     dld_str: dld_str_s,
-    link: datalink_id_t,
 }
 
 impl DldStream {
-    /// Register promiscuous callback to receive packets on the underlying MAC.
-    pub fn add_promisc(
-        self: &Arc<Self>,
-        ptype: mac_client_promisc_type_t,
-        promisc_fn: mac_rx_fn,
-        flags: u16,
-    ) -> Result<MacPromiscHandle<Self>, c_int> {
-        let Some(inner) = self.inner.as_ref() else {
-            return Err(-1);
-        };
-
-        let mut mph = ptr::null_mut();
-
-        // `MacPromiscHandle` keeps a reference to this `MacClientHandle`
-        // until it is removed and so we can safely access it from the
-        // callback via the `arg` pointer.
-        let _parent = self.clone();
-        let parent = Arc::into_raw(_parent) as *mut _;
-        let arg = parent as *mut c_void;
-        let mch = inner.dld_str.ds_mch;
-        let ret = unsafe {
-            // NOTE: arg is reinterpreted as `mac_resource_handle` -> `mac_ring`
-            // in `mac_rx_common`. Is what we've been doing here and before even safe?
-            mac_promisc_add(mch, ptype, promisc_fn, arg, &mut mph, flags)
-        };
-
-        if ret == 0 {
-            Ok(MacPromiscHandle { mph, parent })
-        } else {
-            Err(ret)
-        }
+    pub fn link_id(&self) -> LinkId {
+        self.link
     }
 
     pub fn tx_drop_on_no_desc(
@@ -180,9 +200,9 @@ impl DldStream {
     // XXX: cleanup REQUIRES that we hold the MAC perimeter handle.
     pub fn release(mut self, mph: &MacPerimeterHandle) {
         if let Some(mut inner) = self.inner.take() {
-            if mph.linkid() != inner.link {
-                opte::engine::err!("Tried to free link hold with the wrong MAC perimeter: saw {}, wanted {}",
-                    mph.linkid(), inner.link);
+            if mph.link_id() != self.link {
+                opte::engine::err!("Tried to free link hold with the wrong MAC perimeter: saw {:?}, wanted {:?}",
+                    mph.link_id(), self.link);
             }
             unsafe {
                 dls_close(&mut inner.dld_str);
@@ -191,12 +211,22 @@ impl DldStream {
     }
 }
 
+impl MacClient for DldStream {
+    fn mac_client_handle(&self) -> Result<*mut mac_client_handle, c_int> {
+        let Some(inner) = self.inner.as_ref() else {
+            return Err(-1);
+        };
+
+        Ok(inner.dld_str.ds_mch)
+    }
+}
+
 impl Drop for DldStream {
     fn drop(&mut self) {
-        if let Some(inner) = self.inner.take() {
+        if self.inner.take().is_some() {
             opte::engine::err!(
-                "dropped stream on link {} without releasing!!",
-                inner.link
+                "dropped stream on link {:?} without releasing!!",
+                self.link,
             );
         }
     }

xde/src/mac/mod.rs

@@ -10,6 +10,7 @@
 //! the moment out of laziness.
 pub mod sys;
 
+use crate::dls::LinkId;
 use alloc::ffi::CString;
 use alloc::string::String;
 use alloc::string::ToString;
@@ -198,37 +199,6 @@ impl MacClientHandle {
         }
     }
 
-    /// Register promiscuous callback to receive packets on the underlying MAC.
-    pub fn add_promisc(
-        self: &Arc<Self>,
-        ptype: mac_client_promisc_type_t,
-        promisc_fn: mac_rx_fn,
-        flags: u16,
-    ) -> Result<MacPromiscHandle<Self>, c_int> {
-        let mut mph = ptr::null_mut();
-
-        // `MacPromiscHandle` keeps a reference to this `MacClientHandle`
-        // until it is removed and so we can safely access it from the
-        // callback via the `arg` pointer.
-        let mch = Arc::into_raw(self.clone());
-        let ret = unsafe {
-            mac_promisc_add(
-                self.mch,
-                ptype,
-                promisc_fn,
-                mch as *mut c_void,
-                &mut mph,
-                flags,
-            )
-        };
-
-        if ret == 0 {
-            Ok(MacPromiscHandle { mph, parent: mch })
-        } else {
-            Err(ret)
-        }
-    }
-
     /// Send the [`Packet`] on this client.
     ///
     /// If the packet cannot be sent, return it. If you want to drop
@@ -300,15 +270,52 @@ impl Drop for MacClientHandle {
     }
 }
 
+pub trait MacClient {
+    fn mac_client_handle(&self) -> Result<*mut mac_client_handle, c_int>;
+}
+
+impl MacClient for MacClientHandle {
+    fn mac_client_handle(&self) -> Result<*mut mac_client_handle, c_int> {
+        Ok(self.mch)
+    }
+}
+
 /// Safe wrapper around a `mac_promisc_handle_t`.
 #[derive(Debug)]
 pub struct MacPromiscHandle<P> {
     /// The underlying `mac_promisc_handle_t`.
-    pub(crate) mph: *mut mac_promisc_handle,
+    mph: *mut mac_promisc_handle,
+
+    /// The parent used to create this promiscuous callback.
+    parent: *const P,
+}
+
+impl<P: MacClient> MacPromiscHandle<P> {
+    /// Register a promiscuous callback to receive packets on the underlying MAC.
+    pub fn new(
+        parent: Arc<P>,
+        ptype: mac_client_promisc_type_t,
+        promisc_fn: mac_rx_fn,
+        flags: u16,
+    ) -> Result<MacPromiscHandle<P>, c_int> {
+        let mut mph = ptr::null_mut();
+        let mch = parent.mac_client_handle()?;
+        let parent = Arc::into_raw(parent);
+        let arg = parent as *mut c_void;
+
+        // SAFETY: `MacPromiscHandle` keeps a reference to this `P`
+        // until it is removed and so we can safely access it from the
+        // callback via the `arg` pointer.
+        let ret = unsafe {
+            mac_promisc_add(mch, ptype, promisc_fn, arg, &mut mph, flags)
+        };
 
-    /// The `MacClientHandle` used to create this promiscuous callback.
-    /// MUST BE A RAW ARC.
-    pub(crate) parent: *const P,
+        if ret == 0 {
+            Ok(Self { mph, parent })
+        } else {
+            Err(ret)
+        }
+    }
 }
 
 impl<P> Drop for MacPromiscHandle<P> {
@@ -342,26 +349,26 @@ impl Drop for MacUnicastHandle {
     }
 }
 
-// XXX: cleanup
-
 /// Safe wrapper around a `mac_perim_handle_t`.
 pub struct MacPerimeterHandle {
     mph: mac_perim_handle,
-    link: datalink_id_t,
+    link: LinkId,
 }
 
 impl MacPerimeterHandle {
-    pub fn from_linkid(link: datalink_id_t) -> Result<Self, c_int> {
+    /// Attempt to acquire the MAC perimeter for a given link.
+    pub fn from_linkid(link: LinkId) -> Result<Self, c_int> {
         let mut mph = 0;
-        let res = unsafe { mac_perim_enter_by_linkid(link, &mut mph) };
+        let res = unsafe { mac_perim_enter_by_linkid(link.into(), &mut mph) };
         if res == 0 {
             Ok(Self { mph, link })
         } else {
             Err(res)
         }
     }
 
-    pub fn linkid(&self) -> datalink_id_t {
+    /// Returns the ID of the link whose MAC perimeter is held.
+    pub fn link_id(&self) -> LinkId {
         self.link
     }
 }