Re: [PATCH V6 3/8] libgpiod: Add rust wrapper crate

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 29-09-22, 15:55, Miguel Ojeda wrote:
> It looks like a container whose elements get invalidated, so
> `read_edge_event` could require an exclusive reference to `buffer` in
> Rust, that way you cannot keep borrows to its elements like `ev` if
> you want to call it. But of course this requires tying the lifetime of
> the events to that of the buffer.

What about the below code changes on top of V6 ?

Changes:
- Removed BufferInternal.
- Event contains a reference to the Buffer now, with lifetime.
- read_edge_event() expects a mutable reference to buffer, to make it
  exclusive, i.e. disallow any previous Event references to exist at
  compilation itself.

diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs
index ce583916a2e3..f5426459d779 100644
--- a/bindings/rust/libgpiod/src/edge_event.rs
+++ b/bindings/rust/libgpiod/src/edge_event.rs
@@ -3,12 +3,11 @@
 // Copyright 2022 Linaro Ltd. All Rights Reserved.
 //     Viresh Kumar <viresh.kumar@xxxxxxxxxx>
 
-use std::sync::Arc;
 use std::time::Duration;
 
 use vmm_sys_util::errno::Error as Errno;
 
-use super::{edge::event::BufferInternal, gpiod, EdgeKind, Error, Offset, OperationType, Result};
+use super::{edge::event::Buffer, gpiod, EdgeKind, Error, Offset, OperationType, Result};
 
 /// Line edge events handling
 ///
@@ -22,15 +21,15 @@ use super::{edge::event::BufferInternal, gpiod, EdgeKind, Error, Offset, Operati
 /// number of events are being read.
 
 #[derive(Debug, Eq, PartialEq)]
-pub struct Event {
-    ibuffer: Option<Arc<BufferInternal>>,
+pub struct Event<'b> {
+    buffer: Option<&'b Buffer>,
     event: *mut gpiod::gpiod_edge_event,
 }
 
-impl Event {
+impl<'b> Event<'b> {
     /// Get an event stored in the buffer.
-    pub(crate) fn new(ibuffer: &Arc<BufferInternal>, index: u64) -> Result<Self> {
-        let event = unsafe { gpiod::gpiod_edge_event_buffer_get_event(ibuffer.buffer(), index) };
+    pub(crate) fn new(buffer: &'b Buffer, index: u64) -> Result<Self> {
+        let event = unsafe { gpiod::gpiod_edge_event_buffer_get_event(buffer.buffer(), index) };
         if event.is_null() {
             return Err(Error::OperationFailed(
                 OperationType::EdgeEventBufferGetEvent,
@@ -39,7 +38,7 @@ impl Event {
         }
 
         Ok(Self {
-            ibuffer: Some(ibuffer.clone()),
+            buffer: Some(buffer),
             event,
         })
     }
@@ -54,7 +53,7 @@ impl Event {
         }
 
         Ok(Self {
-            ibuffer: None,
+            buffer: None,
             event,
         })
     }
@@ -91,11 +90,11 @@ impl Event {
     }
 }
 
-impl Drop for Event {
+impl<'b> Drop for Event<'b> {
     /// Free the edge event.
     fn drop(&mut self) {
         // Free the event only if a copy is made
-        if self.ibuffer.is_none() {
+        if self.buffer.is_none() {
             unsafe { gpiod::gpiod_edge_event_free(self.event) };
         }
     }
diff --git a/bindings/rust/libgpiod/src/event_buffer.rs b/bindings/rust/libgpiod/src/event_buffer.rs
index e272e7aa9e9d..11c8b5e1d7c9 100644
--- a/bindings/rust/libgpiod/src/event_buffer.rs
+++ b/bindings/rust/libgpiod/src/event_buffer.rs
@@ -4,7 +4,6 @@
 //     Viresh Kumar <viresh.kumar@xxxxxxxxxx>
 
 use std::os::raw::c_ulong;
-use std::sync::Arc;
 
 use vmm_sys_util::errno::Error as Errno;
 
@@ -12,11 +11,11 @@ use super::{edge, gpiod, Error, OperationType, Result};
 
 /// Line edge events buffer
 #[derive(Clone, Debug, Eq, PartialEq)]
-pub(crate) struct BufferInternal {
+pub struct Buffer {
     buffer: *mut gpiod::gpiod_edge_event_buffer,
 }
 
-impl BufferInternal {
+impl Buffer {
     /// Create a new edge event buffer.
     ///
     /// If capacity equals 0, it will be set to a default value of 64. If
@@ -37,36 +36,6 @@ impl BufferInternal {
     pub(crate) fn buffer(&self) -> *mut gpiod::gpiod_edge_event_buffer {
         self.buffer
     }
-}
-
-impl Drop for BufferInternal {
-    /// Free the edge event buffer and release all associated resources.
-    fn drop(&mut self) {
-        unsafe { gpiod::gpiod_edge_event_buffer_free(self.buffer) };
-    }
-}
-
-/// Line edge events buffer
-#[derive(Clone, Debug, Eq, PartialEq)]
-pub struct Buffer {
-    ibuffer: Arc<BufferInternal>,
-}
-
-impl Buffer {
-    /// Create a new edge event buffer.
-    ///
-    /// If capacity equals 0, it will be set to a default value of 64. If
-    /// capacity is larger than 1024, it will be limited to 1024.
-    pub fn new(capacity: usize) -> Result<Self> {
-        Ok(Self {
-            ibuffer: Arc::new(BufferInternal::new(capacity)?),
-        })
-    }
-
-    /// Private helper, Returns gpiod_edge_event_buffer
-    pub(crate) fn buffer(&self) -> *mut gpiod::gpiod_edge_event_buffer {
-        self.ibuffer.buffer()
-    }
 
     /// Get the capacity of the event buffer.
     pub fn capacity(&self) -> usize {
@@ -75,7 +44,7 @@ impl Buffer {
 
     /// Read an event stored in the buffer.
     pub fn event(&self, index: u64) -> Result<edge::Event> {
-        edge::Event::new(&self.ibuffer, index)
+        edge::Event::new(self, index)
     }
 
     /// Get the number of events the buffer contains.
@@ -88,3 +57,10 @@ impl Buffer {
         self.len() == 0
     }
 }
+
+impl Drop for Buffer {
+    /// Free the edge event buffer and release all associated resources.
+    fn drop(&mut self) {
+        unsafe { gpiod::gpiod_edge_event_buffer_free(self.buffer) };
+    }
+}
diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
index 617efaa34d58..e4ff951aef29 100644
--- a/bindings/rust/libgpiod/src/line_request.rs
+++ b/bindings/rust/libgpiod/src/line_request.rs
@@ -218,7 +218,7 @@ impl Request {
     /// Get a number of edge events from a line request.
     ///
     /// This function will block if no event was queued for the line.
-    pub fn read_edge_events(&self, buffer: &edge::event::Buffer) -> Result<u32> {
+    pub fn read_edge_events(&self, buffer: &mut edge::event::Buffer) -> Result<u32> {
         let ret = unsafe {
             gpiod::gpiod_line_request_read_edge_event(
                 self.request,


And here is an example where we get compilation error on the commented
line:

fn main() -> Result<()> {
    let mut lsettings = line::Settings::new()?;
    let lconfig = line::Config::new()?;
    let offsets = vec![1, 2];

    lsettings.set_prop(&[SettingVal::EdgeDetection(Some(Edge::Both))])?;
    lconfig.add_line_settings(&offsets, lsettings)?;

    let path = "/dev/gpiochip0".to_string();
    let chip = Chip::open(&path)?;

    let rconfig = request::Config::new()?;

    let mut buffer = edge::event::Buffer::new(2)?;
    let request = chip.request_lines(&rconfig, &lconfig)?;

    loop {
        match request.wait_edge_event(None) {
            Err(x) => {
                println!("{:?}", x);
                return Err(Error::InvalidArguments);
            }

            Ok(false) => {
                // This shouldn't happen as the call is blocking.
                panic!();
            }
            Ok(true) => (),
        }

        request.read_edge_events(&mut buffer)?;
        let event0 = buffer.event(0)?;
        let event1 = buffer.event(1)?;

        println!("{:?}", (event0.line_offset(), event1.line_offset()));
        drop(event0);

        // This fails to compile
        // request.read_edge_events(&mut buffer)?;

        drop(event1);
        // This compiles fine
        request.read_edge_events(&mut buffer)?;
    }
}

-- 
viresh