Introduce "WithContext" variants of FromRaw and AsRaw

Based on PR comments, the ability to create Rust wrapper types from raw `libudev` pointer types needs to be preserved. Since doing this correctly requires knowing not only the pointer to some `libudev` struct but also the `struct udev*` that was provided as the context pointer when that struct was allocated, I had to introduce `FromRawWithContext` and `AsRawWithContext` and implement them on `Enumerator`, `Device`, and `Builder`.
d0u9 · Sep 19, 2020 · 909f8d3 · 909f8d3
1 parent c132ed9
commit 909f8d3
Show file tree

Hide file tree

Showing 5 changed files with 152 additions and 20 deletions.
diff --git a/src/device.rs b/src/device.rs
@@ -11,7 +11,7 @@ use libc::{c_char, dev_t};
 use Udev;
 use {ffi, util};
 
-use AsRaw;
+use {AsRaw, FromRaw};
 
 /// A structure that provides access to sysfs/kernel devices.
 pub struct Device {
@@ -36,7 +36,7 @@ impl Drop for Device {
     }
 }
 
-as_raw!(Device, device, ffi::udev_device);
+as_ffi_with_context!(Device, device, ffi::udev_device, ffi::udev_device_ref);
 
 impl Device {
     /// Creates a device for a given syspath.
@@ -48,18 +48,20 @@ impl Device {
         // It would be more efficient to allow callers to create just one context and use multiple
         // devices, however that would be an API-breaking change.
         //
-        // When devices are enumerated using an `Enumerator`, it will use `from_syspath_internal`
-        // which can reuse the existing `Udev` context to avoid this extra overhead.
+        // When devices are enumerated using an `Enumerator`, it will use
+        // `from_syspath_with_context` which can reuse the existing `Udev` context to avoid this
+        // extra overhead.
         let udev = Udev::new()?;
 
-        Self::from_syspath_internal(udev, syspath)
+        Self::from_syspath_with_context(udev, syspath)
     }
 
-    /// Creates a device for a given syspath, re-using an existing `Udev` context
+    /// Creates a device for a given syspath, using an existing `Udev` instance rather than
+    /// creating one automatically.
     ///
     /// The `syspath` parameter should be a path to the device file within the `sysfs` file system,
     /// e.g., `/sys/devices/virtual/tty/tty0`.
-    pub(crate) fn from_syspath_internal(udev: Udev, syspath: &Path) -> Result<Self> {
+    pub fn from_syspath_with_context(udev: Udev, syspath: &Path) -> Result<Self> {
         let syspath = util::os_str_to_cstring(syspath)?;
 
         let ptr = try_alloc!(unsafe {

diff --git a/src/enumerator.rs b/src/enumerator.rs
@@ -5,7 +5,7 @@ use std::path::Path;
 use Udev;
 use {ffi, util};
 
-use {AsRaw, Device};
+use {AsRaw, AsRawWithContext, Device, FromRaw};
 
 /// An enumeration context.
 ///
@@ -32,7 +32,12 @@ impl Drop for Enumerator {
     }
 }
 
-as_raw!(Enumerator, enumerator, ffi::udev_enumerate);
+as_ffi_with_context!(
+    Enumerator,
+    enumerator,
+    ffi::udev_enumerate,
+    ffi::udev_enumerate_ref
+);
 
 impl Enumerator {
     /// Creates a new Enumerator.
@@ -198,8 +203,7 @@ impl Iterator for Devices {
 
             self.entry = unsafe { ffi::udev_list_entry_get_next(self.entry) };
 
-            println!("{}", syspath.display());
-            match Device::from_syspath_internal(self.enumerator.udev.clone(), syspath) {
+            match Device::from_syspath_with_context(self.enumerator.udev.clone(), syspath) {
                 Ok(d) => return Some(d),
                 Err(_) => continue,
             };
@@ -216,12 +220,26 @@ impl Iterator for Devices {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::{AsRawWithContext, FromRawWithContext};
 
     #[test]
     fn create_enumerator() {
         Enumerator::new().unwrap();
     }
 
+    #[test]
+    fn round_trip_to_raw_pointers() {
+        let enumerator = Enumerator::new().unwrap();
+
+        // Round-trip this to raw pointers and back again
+        let (udev, ptr) = enumerator.into_raw_with_context();
+
+        let mut enumerator = unsafe { Enumerator::from_raw_with_context(udev, ptr) };
+
+        // Everything should still work just the same after round-tripping
+        let _ = enumerator.scan_devices().unwrap().collect::<Vec<_>>();
+    }
+
     #[test]
     fn test_enumeration() {
         fn find_hidraws() -> Devices {

diff --git a/src/lib.rs b/src/lib.rs
@@ -12,7 +12,7 @@ extern crate mio;
 pub use device::{Attribute, Attributes, Device, Properties, Property};
 pub use enumerator::{Devices, Enumerator};
 pub use monitor::{Builder as MonitorBuilder, Event, EventType, Socket as MonitorSocket};
-pub(crate) use udev::Udev;
+pub use udev::Udev;
 
 macro_rules! try_alloc {
     ($exp:expr) => {{
@@ -32,41 +32,92 @@ pub trait AsRaw<T: 'static> {
     ///
     /// The reference count will not be increased.
     fn as_raw(&self) -> *mut T;
+
     /// Convert the object into the underlying pointer.
     ///
     /// You are responsible for freeing the object.
     fn into_raw(self) -> *mut T;
 }
 
+/// Receive the underlying raw pointer for types with an associated `udev` struct which must
+/// outlive them.
+pub trait AsRawWithContext<T: 'static> {
+    /// Get a reference of the underlying struct.
+    ///
+    /// The reference count will not be increased.
+    fn as_raw(&self) -> *mut T;
+
+    /// The `udev` context with which this struct was created.  This must live at least as long as
+    /// the struct itself or undefined behavior will result.
+    fn udev(&self) -> &Udev;
+
+    /// Convert the object into the raw `udev` pointer and the underlying pointer for this object.
+    ///
+    /// You are responsible for freeing both.  You're also responsible for ensuring that the `udev`
+    /// pointer is not freed until after this object's pointer is freed.
+    fn into_raw_with_context(self) -> (*mut ffi::udev, *mut T);
+}
+
 /// Convert from a raw pointer
 pub trait FromRaw<T: 'static> {
     /// Create an object from a given raw pointer.
     ///
     /// The reference count will not be increased, be sure not to free this pointer.
     ///
-    /// ## Unsafety
+    /// ## Safety
     ///
     /// The pointer has to be a valid reference to the expected underlying udev-struct or undefined
     /// behaviour might occur.
     unsafe fn from_raw(ptr: *mut T) -> Self;
 }
 
+/// Convert from a raw pointer for types which must be associated with a `Udev` context object.
+pub trait FromRawWithContext<T: 'static> {
+    /// Create an object from a given raw pointer and `udev` context pointer.
+    ///
+    /// The reference count will not be increased, be sure not to free this pointer.
+    ///
+    /// ## Safety
+    ///
+    /// The `udev` pointer must correspond to the `udev` pointer used when `ptr` was created.  If
+    /// not memory corruption and undefined behavior will result.
+    ///
+    /// Both the `udev` and `ptr` pointers must be a valid reference to the expected underlying udev-struct or undefined
+    /// behaviour might occur.  Do NOT attempt to free either pointer; `udev_unref` and the
+    /// corresponding `*_unref` function for `ptr` will be called automatically when this type is
+    /// dropped.
+    unsafe fn from_raw_with_context(udev: *mut ffi::udev, ptr: *mut T) -> Self;
+}
+
 /// Convert from a raw pointer and the matching context
 macro_rules! as_ffi {
-    ($struct_:ident, $field:ident, $type_:ty) => {
-        as_raw!($struct_, $field, $type_);
+    ($struct_:ident, $field:ident, $type_:ty, $ref:path) => {
+        as_raw!($struct_, $field, $type_, $ref);
         from_raw!($struct_, $field, $type_);
     };
 }
 
+macro_rules! as_ffi_with_context {
+    ($struct_:ident, $field:ident, $type_:ty, $ref:path) => {
+        as_raw_with_context!($struct_, $field, $type_, $ref);
+        from_raw_with_context!($struct_, $field, $type_);
+    };
+}
+
 macro_rules! as_raw {
-    ($struct_:ident, $field:ident, $type_:ty) => {
+    ($struct_:ident, $field:ident, $type_:ty, $ref:path) => {
         impl $crate::AsRaw<$type_> for $struct_ {
             fn as_raw(&self) -> *mut $type_ {
                 self.$field
             }
 
             fn into_raw(self) -> *mut $type_ {
+                // Note that all `AsRaw` implementations also implement `Drop` which calls the
+                // `_unref` function that correponds to $type_.  We can't prevent this from
+                // happening, so we have to add a reference here to ensure the returned pointer
+                // remains allocated for the caller.
+                unsafe { $ref(self.$field) };
+
                 self.$field
             }
         }
@@ -83,6 +134,55 @@ macro_rules! from_raw {
     };
 }
 
+macro_rules! as_raw_with_context {
+    ($struct_:ident, $field:ident, $type_:ty, $ref:path) => {
+        impl $crate::AsRawWithContext<$type_> for $struct_ {
+            fn as_raw(&self) -> *mut $type_ {
+                self.$field
+            }
+
+            fn udev(&self) -> &Udev {
+                &self.udev
+            }
+
+            fn into_raw_with_context(self) -> (*mut ffi::udev, *mut $type_) {
+                // We can't call `self.udev.into_raw()` here, because that will consume
+                // `self.udev`, which is not possible because every type that implements
+                // `AsRawWithContext` also implements `Drop`.  Of course we know that it would be
+                // safe here to just skip the `drop()` on `Udev` and "leak" the `udev` pointer back
+                // to the caller, but the Rust compiler doesn't know that.
+                //
+                // So instead we have to add a new reference to the `udev` pointer before we return
+                // it, because as soon as we leave the scope of this function the `Udev` struct
+                // will be dropped which will call `udev_unref` on it.  If there's only once
+                // reference left that will free the pointer and we'll end up returning a dangling
+                // pointer to the caller.
+                //
+                // For much the same reason, we do the same with the pointer of type $type
+                let udev = self.udev.as_raw();
+                unsafe { ffi::udev_ref(udev) };
+
+                unsafe { $ref(self.$field) };
+
+                (udev, self.$field)
+            }
+        }
+    };
+}
+
+macro_rules! from_raw_with_context {
+    ($struct_:ident, $field:ident, $type_:ty) => {
+        impl $crate::FromRawWithContext<$type_> for $struct_ {
+            unsafe fn from_raw_with_context(udev: *mut ffi::udev, t: *mut $type_) -> Self {
+                Self {
+                    udev: Udev::from_raw(udev),
+                    $field: t,
+                }
+            }
+        }
+    };
+}
+
 mod device;
 mod enumerator;
 mod monitor;

diff --git a/src/monitor.rs b/src/monitor.rs
@@ -12,7 +12,7 @@ use mio::{event::Evented, unix::EventedFd, Poll, PollOpt, Ready, Token};
 use Udev;
 use {ffi, util};
 
-use {AsRaw, Device};
+use {AsRaw, AsRawWithContext, Device, FromRaw};
 
 /// Monitors for device events.
 ///
@@ -41,7 +41,7 @@ impl Drop for Builder {
     }
 }
 
-as_raw!(Builder, monitor, ffi::udev_monitor);
+as_ffi_with_context!(Builder, monitor, ffi::udev_monitor, ffi::udev_monitor_ref);
 
 impl Builder {
     /// Creates a new `Monitor`.

diff --git a/src/udev.rs b/src/udev.rs
@@ -15,7 +15,7 @@ use FromRaw;
 /// `udev` is a ref-counted struct, with references added and removed with `udev_ref` and
 /// `udef_unref` respectively.  This Rust wrapper takes advantage of that ref counting to implement
 /// `Clone` and `Drop`, so callers need not worry about any C-specific resource management.
-pub(crate) struct Udev {
+pub struct Udev {
     udev: *mut ffi::udev,
 }
 
@@ -31,7 +31,7 @@ impl Drop for Udev {
     }
 }
 
-as_ffi!(Udev, udev, ffi::udev);
+as_ffi!(Udev, udev, ffi::udev, ffi::udev_ref);
 
 impl Udev {
     /// Creates a new Udev context.
@@ -61,4 +61,16 @@ mod tests {
             udev = clone;
         }
     }
+
+    #[test]
+    fn round_trip_to_raw_pointers() {
+        // Make sure this can be made into a raw pointer, then back to a Rust type, and still works
+        let udev = Udev::new().unwrap();
+
+        let ptr = udev.into_raw();
+
+        let udev = unsafe { Udev::from_raw(ptr) };
+
+        assert_eq!(ptr, udev.as_raw());
+    }
 }