Re: [PATCH V7 0/8] libgpiod: Add Rust bindings

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

Hi Björn,

I have bounced (mutt's feature) the initial emails to your and other
email ids that Miguel added. The patches should be in your inbox now.

On 20-10-22, 13:29, Björn Roy Baron wrote:
> At
> https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/lib.rs#L469
> and elsewhere you might want to use `CStr::from_ptr(version)`. This
> does the `strlen` call for you and you can convert it to an `&str`
> using `.to_str()`.
 
> At
> https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/chip.rs#L171
> you could use `CString` and use the `.as_ptr()` method to get a
> null-terminated string. Not sure if it would be nicer that what you
> currently have though.

These two were nice. Thanks.

> At
> https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/edge_event.rs#L46
> the lifetimes are unclear. Is Event allowed to outlive the buffer?
> Can you add a lifetime annotation like fn event_clone<'a>(event:
> &Event<'a>) -> Result<Event<'a>> if it isn't allowed to outlive the
> buffer or fn event_clone<'a, 'b>(event: &Event<'a>) ->
> Result<Event<'b>> if it is allowed to outlive the buffer. I'm not
> sure which of the two the lifetime elision rules cause the current
> code to be equivalent of, but even if it is correct, explicitly
> stating the lifetime here is clearer IMHO.

An Event created using Event::new() can't outlive the buffer, though
an Event created using Event::event_clone() can.

I tried to play around it based on your suggestion and here is the
diff, does it make sense ?

diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs
index b36c23601bb4..0d328ebb2b03 100644
--- a/bindings/rust/libgpiod/src/edge_event.rs
+++ b/bindings/rust/libgpiod/src/edge_event.rs
@@ -33,7 +33,7 @@ pub struct Event<'b> {

 impl<'b> Event<'b> {
     /// Get an event stored in the buffer.
-    pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Self> {
+    pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Event<'b>> {
         // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
         // as the `struct Event`.
         let event = unsafe {
@@ -52,22 +52,6 @@ impl<'b> Event<'b> {
         })
     }

-    pub fn event_clone(event: &Event) -> Result<Self> {
-        // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
-        let event = unsafe { gpiod::gpiod_edge_event_copy(event.event) };
-        if event.is_null() {
-            return Err(Error::OperationFailed(
-                OperationType::EdgeEventCopy,
-                Errno::last(),
-            ));
-        }
-
-        Ok(Self {
-            buffer: None,
-            event,
-        })
-    }
-
     /// Get the event type.
     pub fn event_type(&self) -> Result<EdgeKind> {
         // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
@@ -105,6 +89,27 @@ impl<'b> Event<'b> {
     }
 }

+impl<'e, 'b> Event<'e> {
+    pub fn event_clone(event: &Event<'b>) -> Result<Event<'e>>
+    where
+        'e: 'b,
+    {
+        // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
+        let event = unsafe { gpiod::gpiod_edge_event_copy(event.event) };
+        if event.is_null() {
+            return Err(Error::OperationFailed(
+                OperationType::EdgeEventCopy,
+                Errno::last(),
+            ));
+        }
+
+        Ok(Self {
+            buffer: None,
+            event,
+        })
+    }
+}
+

-- 
viresh