[libgpiod][PATCH 0/3] bindings: rust: fix modeling of line_info lifetimes

[libgpiod][PATCH 0/3] bindings: rust: fix modeling of line_info lifetimes

[Erik Schilling]

While reviewing the bindings for thread-safety, I realized that the
bindings did not properly model the lifetimes of non-owned line_info
instances.

This fixes that. It might be a bit might bending. I tried to provide
lengthy comments to clarify what happens.

To: Linux-GPIO <linux-gpio@vger.kernel.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
Erik Schilling (3):
      bindings: rust: fix soundness of line_info modeling
      bindings: rust: allow cloning line::Info -> line::OwnedInfo
      bindings: rust: bump major for libgpiod crate

 bindings/rust/libgpiod/Cargo.toml         |   2 +-
 bindings/rust/libgpiod/src/chip.rs        |  16 +++-
 bindings/rust/libgpiod/src/info_event.rs  |   6 +-
 bindings/rust/libgpiod/src/lib.rs         |   1 +
 bindings/rust/libgpiod/src/line_info.rs   | 138 ++++++++++++++++++++++--------
 bindings/rust/libgpiod/tests/line_info.rs |  53 ++++++++++++
 6 files changed, 171 insertions(+), 45 deletions(-)
---
base-commit: ced90e79217793957b11414f47f8aa8a77c7a2d5
change-id: 20230927-rust-line-info-soundness-14c08e0d26e9

Best regards,
-- 
Erik Schilling <erik.schilling@linaro.org>

[libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Erik Schilling]

While attention was provided to prevent freeing in non-owned use-cases,
the lifetime of these object was not properly modeled.

The line_info from an event may only be used for as long as the event
exists.

This allowed us to write unsafe-free Rust code that causes a
use-after-free:

  let event = chip.read_info_event().unwrap();
  let line_info = event.line_info().unwrap();
  drop(event);
  dbg!(line_info.name().unwrap());

Which makes the AddressSanitizer scream:

  ==90154==ERROR: AddressSanitizer: heap-use-after-free on address 0x50b000005dc4 at pc 0x55a4f883a009 bp 0x7f60ac8fbbc0 sp 0x7f60ac8fb388
  READ of size 2 at 0x50b000005dc4 thread T2
      [...]
      #3 0x55a4f8c3d5f3 in libgpiod::line_info::Info::name::h5ba0bfd360ecb405 libgpiod/bindings/rust/libgpiod/src/line_info.rs:70:18
    	[...]

  0x50b000005dc4 is located 4 bytes inside of 112-byte region [0x50b000005dc0,0x50b000005e30)
  freed by thread T2 here:
      [...]
      #1 0x7f60b07f7e31 in gpiod_info_event_free libgpiod/lib/info-event.c:61:2
      [...]

  previously allocated by thread T2 here:
      #0 0x55a4f88b04be in malloc /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
      #1 0x7f60b07f8ff0 in gpiod_line_info_from_uapi libgpiod/lib/line-info.c:144:9

The fix is to distinguish between the owned and non-owned variants and
assigning lifetimes to non-owned variants.

For modeling the non-owned type there are a couple of options. The ideal
solution would be using extern_types [1]. But that is still unstable.
Instead, we are defining a #[repr(transparent)] wrapper around the opaque
gpiod_line_info struct and cast the pointer to a reference.

This was recommended on the Rust Discord server as good practise.
(Thanks to Kyuuhachi, shepmaster, pie_flavor and ilyvion! Also thanks to
@epilys for a brainstorming on this on #linaro-virtualization IRC).

Of course, determining the lifetimes and casting across the types
requires some care. So this adds a couple of SAFETY comments that would
probably also have helped the existing code.

[1] https://github.com/rust-lang/rfcs/blob/master/text/1861-extern-types.md

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
 bindings/rust/libgpiod/src/chip.rs       |  16 +++-
 bindings/rust/libgpiod/src/info_event.rs |   6 +-
 bindings/rust/libgpiod/src/line_info.rs  | 128 +++++++++++++++++++++----------
 3 files changed, 103 insertions(+), 47 deletions(-)

diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
index 81e1be6..02265fc 100644
--- a/bindings/rust/libgpiod/src/chip.rs
+++ b/bindings/rust/libgpiod/src/chip.rs
@@ -95,7 +95,7 @@ impl Chip {
     }
 
     /// Get a snapshot of information about the line.
-    pub fn line_info(&self, offset: Offset) -> Result<line::Info> {
+    pub fn line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
         // SAFETY: The `gpiod_line_info` returned by libgpiod is guaranteed to live as long
         // as the `struct Info`.
         let info = unsafe { gpiod::gpiod_chip_get_line_info(self.ichip.chip, offset) };
@@ -107,12 +107,16 @@ impl Chip {
             ));
         }
 
-        line::Info::new(info)
+        // SAFETY: We verified that the pointer is valid. We own the pointer and
+        // no longer use it after converting it into a InfoOwned instance.
+        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
+
+        Ok(line_info)
     }
 
     /// Get the current snapshot of information about the line at given offset and start watching
     /// it for future changes.
-    pub fn watch_line_info(&self, offset: Offset) -> Result<line::Info> {
+    pub fn watch_line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
         let info = unsafe { gpiod::gpiod_chip_watch_line_info(self.ichip.chip, offset) };
 
@@ -123,7 +127,11 @@ impl Chip {
             ));
         }
 
-        line::Info::new_watch(info)
+        // SAFETY: We verified that the pointer is valid. We own the instance and
+        // no longer use it after converting it into a InfoOwned instance.
+        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
+
+        Ok(line_info)
     }
 
     /// Stop watching a line
diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs
index db60600..e88dd72 100644
--- a/bindings/rust/libgpiod/src/info_event.rs
+++ b/bindings/rust/libgpiod/src/info_event.rs
@@ -44,7 +44,7 @@ impl Event {
     }
 
     /// Get the line-info object associated with the event.
-    pub fn line_info(&self) -> Result<line::Info> {
+    pub fn line_info(&self) -> Result<&line::Info> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
         let info = unsafe { gpiod::gpiod_info_event_get_line_info(self.event) };
 
@@ -55,7 +55,9 @@ impl Event {
             ));
         }
 
-        line::Info::new_from_event(info)
+        let line_info = unsafe { line::Info::from_raw_non_owning(info) };
+
+        Ok(line_info)
     }
 }
 
diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
index c4f488c..32c4bb2 100644
--- a/bindings/rust/libgpiod/src/line_info.rs
+++ b/bindings/rust/libgpiod/src/line_info.rs
@@ -2,9 +2,10 @@
 // SPDX-FileCopyrightText: 2022 Linaro Ltd.
 // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
 
-use std::ffi::CStr;
+use std::ops::Deref;
 use std::str;
 use std::time::Duration;
+use std::{ffi::CStr, marker::PhantomData};
 
 use super::{
     gpiod,
@@ -12,7 +13,7 @@ use super::{
     Error, Result,
 };
 
-/// Line info
+/// Line info reference
 ///
 /// Exposes functions for retrieving kernel information about both requested and
 /// free lines.  Line info object contains an immutable snapshot of a line's status.
@@ -20,48 +21,57 @@ use super::{
 /// The line info contains all the publicly available information about a
 /// line, which does not include the line value.  The line must be requested
 /// to access the line value.
-
-#[derive(Debug, Eq, PartialEq)]
+///
+/// [Info] only abstracts a reference to a [gpiod::gpiod_line_info] instance whose lifetime is managed
+/// by a different object instance. The owned counter-part of this type is [InfoOwned].
+#[derive(Debug)]
+#[repr(transparent)]
 pub struct Info {
-    info: *mut gpiod::gpiod_line_info,
-    contained: bool,
+    _info: gpiod::gpiod_line_info,
+    // Avoid the automatic `Sync` implementation.
+    //
+    // The C lib does not allow parallel invocations of the API. We could model
+    // that by restricting all wrapper functions to `&mut Info` - which would
+    // ensure exclusive access. But that would make the API a bit weird...
+    // So instead we just suppress the `Sync` implementation, which suppresses
+    // the `Send` implementation on `&Info` - disallowing to send it to other
+    // threads, making concurrent use impossible.
+    _not_sync: PhantomData<*mut gpiod::gpiod_line_info>,
 }
 
 impl Info {
-    fn new_internal(info: *mut gpiod::gpiod_line_info, contained: bool) -> Result<Self> {
-        Ok(Self { info, contained })
-    }
-
-    /// Get a snapshot of information about the line.
-    pub(crate) fn new(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
-        Info::new_internal(info, false)
-    }
-
-    /// Get a snapshot of information about the line and start watching it for changes.
-    pub(crate) fn new_watch(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
-        Info::new_internal(info, false)
+    /// Converts a non-owning pointer to a wrapper reference of a specific
+    /// lifetime
+    ///
+    /// No ownership will be assumed, the pointer must be free'd by the original
+    /// owner.
+    ///
+    /// SAFETY: The pointer must point to an instance that is valid for the
+    /// entire lifetime 'a. The instance must be owned by an object that is
+    /// owned by the thread invoking this method. The owning object may not be
+    /// moved to another thread for the entire lifetime 'a.
+    pub(crate) unsafe fn from_raw_non_owning<'a>(info: *mut gpiod::gpiod_line_info) -> &'a Info {
+        &*(info as *mut _)
     }
 
-    /// Get the Line info object associated with an event.
-    pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
-        Info::new_internal(info, true)
+    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
+        self as *const _ as *mut _
     }
 
     /// Get the offset of the line within the GPIO chip.
     ///
     /// The offset uniquely identifies the line on the chip. The combination of the chip and offset
     /// uniquely identifies the line within the system.
-
     pub fn offset(&self) -> Offset {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        unsafe { gpiod::gpiod_line_info_get_offset(self.info) }
+        unsafe { gpiod::gpiod_line_info_get_offset(self.as_raw_ptr()) }
     }
 
     /// Get GPIO line's name.
     pub fn name(&self) -> Result<&str> {
         // SAFETY: The string returned by libgpiod is guaranteed to live as long
         // as the `struct Info`.
-        let name = unsafe { gpiod::gpiod_line_info_get_name(self.info) };
+        let name = unsafe { gpiod::gpiod_line_info_get_name(self.as_raw_ptr()) };
         if name.is_null() {
             return Err(Error::NullString("GPIO line's name"));
         }
@@ -79,14 +89,14 @@ impl Info {
     /// the line is used and we can't request it.
     pub fn is_used(&self) -> bool {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        unsafe { gpiod::gpiod_line_info_is_used(self.info) }
+        unsafe { gpiod::gpiod_line_info_is_used(self.as_raw_ptr()) }
     }
 
     /// Get the GPIO line's consumer name.
     pub fn consumer(&self) -> Result<&str> {
         // SAFETY: The string returned by libgpiod is guaranteed to live as long
         // as the `struct Info`.
-        let name = unsafe { gpiod::gpiod_line_info_get_consumer(self.info) };
+        let name = unsafe { gpiod::gpiod_line_info_get_consumer(self.as_raw_ptr()) };
         if name.is_null() {
             return Err(Error::NullString("GPIO line's consumer name"));
         }
@@ -100,44 +110,44 @@ impl Info {
     /// Get the GPIO line's direction.
     pub fn direction(&self) -> Result<Direction> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.info) })
+        Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.as_raw_ptr()) })
     }
 
     /// Returns true if the line is "active-low", false otherwise.
     pub fn is_active_low(&self) -> bool {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        unsafe { gpiod::gpiod_line_info_is_active_low(self.info) }
+        unsafe { gpiod::gpiod_line_info_is_active_low(self.as_raw_ptr()) }
     }
 
     /// Get the GPIO line's bias setting.
     pub fn bias(&self) -> Result<Option<Bias>> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.info) })
+        Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.as_raw_ptr()) })
     }
 
     /// Get the GPIO line's drive setting.
     pub fn drive(&self) -> Result<Drive> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.info) })
+        Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.as_raw_ptr()) })
     }
 
     /// Get the current edge detection setting of the line.
     pub fn edge_detection(&self) -> Result<Option<Edge>> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.info) })
+        Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.as_raw_ptr()) })
     }
 
     /// Get the current event clock setting used for edge event timestamps.
     pub fn event_clock(&self) -> Result<EventClock> {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.info) })
+        EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.as_raw_ptr()) })
     }
 
     /// Returns true if the line is debounced (either by hardware or by the
     /// kernel software debouncer), false otherwise.
     pub fn is_debounced(&self) -> bool {
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-        unsafe { gpiod::gpiod_line_info_is_debounced(self.info) }
+        unsafe { gpiod::gpiod_line_info_is_debounced(self.as_raw_ptr()) }
     }
 
     /// Get the debounce period of the line.
@@ -147,18 +157,54 @@ impl Info {
         #[allow(clippy::unnecessary_cast)]
         // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
         Duration::from_micros(unsafe {
-            gpiod::gpiod_line_info_get_debounce_period_us(self.info) as u64
+            gpiod::gpiod_line_info_get_debounce_period_us(self.as_raw_ptr()) as u64
         })
     }
 }
 
-impl Drop for Info {
+/// Line info
+///
+/// This is the owned counterpart to [Info]. Due to a [Deref] implementation,
+/// all functions of [Info] can also be called on this type.
+#[derive(Debug)]
+pub struct InfoOwned {
+    info: *mut gpiod::gpiod_line_info,
+}
+
+// SAFETY: InfoOwned models a owned instance whose ownership may be safely
+// transferred to other threads.
+unsafe impl Send for InfoOwned {}
+
+impl InfoOwned {
+    /// Converts a owned pointer into an owned instance
+    ///
+    /// Assumes sole ownership over a [gpiod::gpiod_line_info] instance.
+    ///
+    /// SAFETY: The pointer must point to an instance that is valid. After
+    /// constructing an [InfoOwned] the pointer MUST NOT be used for any other
+    /// purpose anymore. All interactions with the libgpiod API have to happen
+    /// through this object.
+    pub(crate) unsafe fn from_raw_owned(info: *mut gpiod::gpiod_line_info) -> InfoOwned {
+        InfoOwned { info }
+    }
+}
+
+impl Deref for InfoOwned {
+    type Target = Info;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: The pointer is valid for the entire lifetime '0. InfoOwned is
+        // not Sync. Therefore, no &InfoOwned may be held by a different thread.
+        // Hence, the current thread owns it. Since we borrow with the lifetime
+        // of '0, no move to a different thread can occur while a reference
+        // remains being hold.
+        unsafe { Info::from_raw_non_owning(self.info) }
+    }
+}
+
+impl Drop for InfoOwned {
     fn drop(&mut self) {
-        // We must not free the Line info object created from `struct chip::Event` by calling
-        // libgpiod API.
-        if !self.contained {
-            // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
-            unsafe { gpiod::gpiod_line_info_free(self.info) }
-        }
+        // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
+        unsafe { gpiod::gpiod_line_info_free(self.info) }
     }
 }

-- 
2.41.0

[libgpiod][PATCH 2/3] bindings: rust: allow cloning line::Info -> line::OwnedInfo

[Erik Schilling]

While one would usually use the ToOwned [1] contract in rust, libgpipd's
API only allows copying that may fail.

Thus, we cannot implement the existing trait and roll our own method. I
went with `try_clone` since that seems to be used in similar cases across
the `std` crate [2].

It also closes the gap of not having any way to clone owned instances.
Though - again - not through the Clone trait which may not fail [3].

[1] https://doc.rust-lang.org/std/borrow/trait.ToOwned.html
[2] https://doc.rust-lang.org/std/index.html?search=try_clone
[3] https://doc.rust-lang.org/std/clone/trait.Clone.html

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
 bindings/rust/libgpiod/src/lib.rs         |  1 +
 bindings/rust/libgpiod/src/line_info.rs   | 16 ++++++++++
 bindings/rust/libgpiod/tests/line_info.rs | 53 +++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)

diff --git a/bindings/rust/libgpiod/src/lib.rs b/bindings/rust/libgpiod/src/lib.rs
index 3acc98c..fd95ed2 100644
--- a/bindings/rust/libgpiod/src/lib.rs
+++ b/bindings/rust/libgpiod/src/lib.rs
@@ -74,6 +74,7 @@ pub enum OperationType {
     LineConfigSetOutputValues,
     LineConfigGetOffsets,
     LineConfigGetSettings,
+    LineInfoCopy,
     LineRequestReconfigLines,
     LineRequestGetVal,
     LineRequestGetValSubset,
diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
index 32c4bb2..fe01a14 100644
--- a/bindings/rust/libgpiod/src/line_info.rs
+++ b/bindings/rust/libgpiod/src/line_info.rs
@@ -58,6 +58,22 @@ impl Info {
         self as *const _ as *mut _
     }
 
+    /// Clones the [gpiod::gpiod_line_info] instance to an [InfoOwned]
+    pub fn try_clone(&self) -> Result<InfoOwned> {
+        // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
+        let copy = unsafe { gpiod::gpiod_line_info_copy(self.as_raw_ptr()) };
+        if copy.is_null() {
+            return Err(Error::OperationFailed(
+                crate::OperationType::LineInfoCopy,
+                errno::errno(),
+            ));
+        }
+
+        // SAFETY: The copy succeeded, we are the owner and stop using the
+        // pointer after this.
+        Ok(unsafe { InfoOwned::from_raw_owned(copy) })
+    }
+
     /// Get the offset of the line within the GPIO chip.
     ///
     /// The offset uniquely identifies the line on the chip. The combination of the chip and offset
diff --git a/bindings/rust/libgpiod/tests/line_info.rs b/bindings/rust/libgpiod/tests/line_info.rs
index ce66a60..d02c9ea 100644
--- a/bindings/rust/libgpiod/tests/line_info.rs
+++ b/bindings/rust/libgpiod/tests/line_info.rs
@@ -19,6 +19,10 @@ mod line_info {
     const NGPIO: usize = 8;
 
     mod properties {
+        use std::thread;
+
+        use libgpiod::{line, request};
+
         use super::*;
 
         #[test]
@@ -271,5 +275,54 @@ mod line_info {
             assert!(info.is_debounced());
             assert_eq!(info.debounce_period(), Duration::from_millis(100));
         }
+
+        fn generate_line_event(chip: &Chip) {
+            let mut line_config = line::Config::new().unwrap();
+            line_config
+                .add_line_settings(&[0], line::Settings::new().unwrap())
+                .unwrap();
+
+            let mut request = chip
+                .request_lines(Some(&request::Config::new().unwrap()), &line_config)
+                .unwrap();
+
+            let mut new_line_config = line::Config::new().unwrap();
+            let mut settings = line::Settings::new().unwrap();
+            settings.set_direction(Direction::Output).unwrap();
+            new_line_config.add_line_settings(&[0], settings).unwrap();
+            request.reconfigure_lines(&new_line_config).unwrap();
+        }
+
+        #[test]
+        fn ownership() {
+            let sim = Sim::new(Some(1), None, false).unwrap();
+            sim.set_line_name(0, "Test line").unwrap();
+            sim.enable().unwrap();
+
+            let chip = Chip::open(&sim.dev_path()).unwrap();
+
+            // start watching line
+            chip.watch_line_info(0).unwrap();
+
+            generate_line_event(&chip);
+
+            // read generated event
+            let event = chip.read_info_event().unwrap();
+            let info = event.line_info().unwrap();
+            assert_eq!(info.name().unwrap(), "Test line");
+
+            // clone info and move to separate thread
+            let info = info.try_clone().unwrap();
+
+            // drop the original event with the associated line_info
+            drop(event);
+
+            // assert that we can still read the name
+            thread::scope(|s| {
+                s.spawn(move || {
+                    assert_eq!(info.name().unwrap(), "Test line");
+                });
+            });
+        }
     }
 }

-- 
2.41.0

[libgpiod][PATCH 3/3] bindings: rust: bump major for libgpiod crate

[Erik Schilling]

Since the type changes around ownership of line_info instances are not
necessarily transparent to the user, we bump the major for the next
release of the crate.

Note:
I am using the term "major" as defined in the Rust SemVer compatibility
guide [1], where the first non-zero digit is considered as "major".

[1] https://doc.rust-lang.org/cargo/reference/semver.html

Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
 bindings/rust/libgpiod/Cargo.toml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bindings/rust/libgpiod/Cargo.toml b/bindings/rust/libgpiod/Cargo.toml
index 518e5e5..3be4aa0 100644
--- a/bindings/rust/libgpiod/Cargo.toml
+++ b/bindings/rust/libgpiod/Cargo.toml
@@ -4,7 +4,7 @@
 
 [package]
 name = "libgpiod"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["Viresh Kumar <viresh.kumar@linaro.org>"]
 description = "libgpiod wrappers"
 repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"

-- 
2.41.0

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Viresh Kumar]

On 27-09-23, 18:29, Erik Schilling wrote:
> diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
> index 81e1be6..02265fc 100644
> --- a/bindings/rust/libgpiod/src/chip.rs
> +++ b/bindings/rust/libgpiod/src/chip.rs
> @@ -95,7 +95,7 @@ impl Chip {
>      }
>  
>      /// Get a snapshot of information about the line.
> -    pub fn line_info(&self, offset: Offset) -> Result<line::Info> {
> +    pub fn line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
>          // SAFETY: The `gpiod_line_info` returned by libgpiod is guaranteed to live as long
>          // as the `struct Info`.
>          let info = unsafe { gpiod::gpiod_chip_get_line_info(self.ichip.chip, offset) };
> @@ -107,12 +107,16 @@ impl Chip {
>              ));
>          }
>  
> -        line::Info::new(info)
> +        // SAFETY: We verified that the pointer is valid. We own the pointer and
> +        // no longer use it after converting it into a InfoOwned instance.
> +        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
> +
> +        Ok(line_info)

Maybe get rid of the extra `line_info` variable and return directly ?

>      }
>  
>      /// Get the current snapshot of information about the line at given offset and start watching
>      /// it for future changes.
> -    pub fn watch_line_info(&self, offset: Offset) -> Result<line::Info> {
> +    pub fn watch_line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
>          // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
>          let info = unsafe { gpiod::gpiod_chip_watch_line_info(self.ichip.chip, offset) };
>  
> @@ -123,7 +127,11 @@ impl Chip {
>              ));
>          }
>  
> -        line::Info::new_watch(info)
> +        // SAFETY: We verified that the pointer is valid. We own the instance and
> +        // no longer use it after converting it into a InfoOwned instance.
> +        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
> +
> +        Ok(line_info)

Same here ?

> diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs
> index db60600..e88dd72 100644
> --- a/bindings/rust/libgpiod/src/info_event.rs
> +++ b/bindings/rust/libgpiod/src/info_event.rs
> @@ -44,7 +44,7 @@ impl Event {
>      }
>  
>      /// Get the line-info object associated with the event.
> -    pub fn line_info(&self) -> Result<line::Info> {
> +    pub fn line_info(&self) -> Result<&line::Info> {
>          // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
>          let info = unsafe { gpiod::gpiod_info_event_get_line_info(self.event) };
>  
> @@ -55,7 +55,9 @@ impl Event {
>              ));
>          }
>  
> -        line::Info::new_from_event(info)
> +        let line_info = unsafe { line::Info::from_raw_non_owning(info) };

SAFETY comment ?

> +
> +        Ok(line_info)
>      }
>  }
>  
> diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
>  impl Info {
> -    fn new_internal(info: *mut gpiod::gpiod_line_info, contained: bool) -> Result<Self> {
> -        Ok(Self { info, contained })
> -    }
> -
> -    /// Get a snapshot of information about the line.
> -    pub(crate) fn new(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> -        Info::new_internal(info, false)
> -    }
> -
> -    /// Get a snapshot of information about the line and start watching it for changes.
> -    pub(crate) fn new_watch(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> -        Info::new_internal(info, false)
> +    /// Converts a non-owning pointer to a wrapper reference of a specific
> +    /// lifetime
> +    ///
> +    /// No ownership will be assumed, the pointer must be free'd by the original
> +    /// owner.
> +    ///
> +    /// SAFETY: The pointer must point to an instance that is valid for the
> +    /// entire lifetime 'a. The instance must be owned by an object that is
> +    /// owned by the thread invoking this method. The owning object may not be
> +    /// moved to another thread for the entire lifetime 'a.
> +    pub(crate) unsafe fn from_raw_non_owning<'a>(info: *mut gpiod::gpiod_line_info) -> &'a Info {

I think we can get rid of _non_owning, and _owned later on, from functions since
the parent structure already says so.

Info::from_raw()
InfoOwned::from_raw()

should be good enough ?

> -    /// Get the Line info object associated with an event.
> -    pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> -        Info::new_internal(info, true)
> +    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> +        self as *const _ as *mut _

What's wrong with keeping `_info` as `info` in the structure and using it
directly instead of this, since this is private anyway ?

-- 
viresh

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Erik Schilling]

On Thu Sep 28, 2023 at 1:27 PM CEST, Viresh Kumar wrote:
> On 27-09-23, 18:29, Erik Schilling wrote:
> > diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
> > index 81e1be6..02265fc 100644
> > --- a/bindings/rust/libgpiod/src/chip.rs
> > +++ b/bindings/rust/libgpiod/src/chip.rs
> > @@ -95,7 +95,7 @@ impl Chip {
> >      }
> >  
> >      /// Get a snapshot of information about the line.
> > -    pub fn line_info(&self, offset: Offset) -> Result<line::Info> {
> > +    pub fn line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
> >          // SAFETY: The `gpiod_line_info` returned by libgpiod is guaranteed to live as long
> >          // as the `struct Info`.
> >          let info = unsafe { gpiod::gpiod_chip_get_line_info(self.ichip.chip, offset) };
> > @@ -107,12 +107,16 @@ impl Chip {
> >              ));
> >          }
> >  
> > -        line::Info::new(info)
> > +        // SAFETY: We verified that the pointer is valid. We own the pointer and
> > +        // no longer use it after converting it into a InfoOwned instance.
> > +        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
> > +
> > +        Ok(line_info)
>
> Maybe get rid of the extra `line_info` variable and return directly ?

Will fix in v2

>
> >      }
> >  
> >      /// Get the current snapshot of information about the line at given offset and start watching
> >      /// it for future changes.
> > -    pub fn watch_line_info(&self, offset: Offset) -> Result<line::Info> {
> > +    pub fn watch_line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
> >          // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> >          let info = unsafe { gpiod::gpiod_chip_watch_line_info(self.ichip.chip, offset) };
> >  
> > @@ -123,7 +127,11 @@ impl Chip {
> >              ));
> >          }
> >  
> > -        line::Info::new_watch(info)
> > +        // SAFETY: We verified that the pointer is valid. We own the instance and
> > +        // no longer use it after converting it into a InfoOwned instance.
> > +        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
> > +
> > +        Ok(line_info)
>
> Same here ?
>
> > diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs
> > index db60600..e88dd72 100644
> > --- a/bindings/rust/libgpiod/src/info_event.rs
> > +++ b/bindings/rust/libgpiod/src/info_event.rs
> > @@ -44,7 +44,7 @@ impl Event {
> >      }
> >  
> >      /// Get the line-info object associated with the event.
> > -    pub fn line_info(&self) -> Result<line::Info> {
> > +    pub fn line_info(&self) -> Result<&line::Info> {
> >          // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> >          let info = unsafe { gpiod::gpiod_info_event_get_line_info(self.event) };
> >  
> > @@ -55,7 +55,9 @@ impl Event {
> >              ));
> >          }
> >  
> > -        line::Info::new_from_event(info)
> > +        let line_info = unsafe { line::Info::from_raw_non_owning(info) };
>
> SAFETY comment ?

Good catch. Forgot that the lint is not enabled by default... Will fix
in v2.

>
> > +
> > +        Ok(line_info)
> >      }
> >  }
> >  
> > diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
> >  impl Info {
> > -    fn new_internal(info: *mut gpiod::gpiod_line_info, contained: bool) -> Result<Self> {
> > -        Ok(Self { info, contained })
> > -    }
> > -
> > -    /// Get a snapshot of information about the line.
> > -    pub(crate) fn new(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> > -        Info::new_internal(info, false)
> > -    }
> > -
> > -    /// Get a snapshot of information about the line and start watching it for changes.
> > -    pub(crate) fn new_watch(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> > -        Info::new_internal(info, false)
> > +    /// Converts a non-owning pointer to a wrapper reference of a specific
> > +    /// lifetime
> > +    ///
> > +    /// No ownership will be assumed, the pointer must be free'd by the original
> > +    /// owner.
> > +    ///
> > +    /// SAFETY: The pointer must point to an instance that is valid for the
> > +    /// entire lifetime 'a. The instance must be owned by an object that is
> > +    /// owned by the thread invoking this method. The owning object may not be
> > +    /// moved to another thread for the entire lifetime 'a.
> > +    pub(crate) unsafe fn from_raw_non_owning<'a>(info: *mut gpiod::gpiod_line_info) -> &'a Info {
>
> I think we can get rid of _non_owning, and _owned later on, from functions since
> the parent structure already says so.
>
> Info::from_raw()
> InfoOwned::from_raw()
>
> should be good enough ?

I got no strong feelings here. I first started with `from_raw`, but switched to
the added suffix since `Info::from_raw` sounded ambigous to me.

>
> > -    /// Get the Line info object associated with an event.
> > -    pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> > -        Info::new_internal(info, true)
> > +    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> > +        self as *const _ as *mut _
>
> What's wrong with keeping `_info` as `info` in the structure and using it
> directly instead of this, since this is private anyway ?

We would still need to cast it the same way. One _could_ write:

    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
        &self.info as *const _ as *mut _
    }

But the cast dance is still required since we need a *mut, but start
with a readonly reference.

This is required since libgpiod's C lib keeps the struct internals
opaque and does not make guarantees about immutable datastructures for
any API calls.

Technically, the 1:1 mapping of this to Rust would be to restrict the
entire API to `&mut self`. One could do that - it would probably allow
us to advertise the structs as `Sync` - but it would require consumers
to declare all libgpiod-related variables as `mut`.

- Erik

Re: [libgpiod][PATCH 2/3] bindings: rust: allow cloning line::Info -> line::OwnedInfo

[Erik Schilling]

On Wed Sep 27, 2023 at 6:29 PM CEST, Erik Schilling wrote:
> While one would usually use the ToOwned [1] contract in rust, libgpipd's
> API only allows copying that may fail.
>
> Thus, we cannot implement the existing trait and roll our own method. I
> went with `try_clone` since that seems to be used in similar cases across
> the `std` crate [2].
>
> It also closes the gap of not having any way to clone owned instances.
> Though - again - not through the Clone trait which may not fail [3].
>
> [1] https://doc.rust-lang.org/std/borrow/trait.ToOwned.html
> [2] https://doc.rust-lang.org/std/index.html?search=try_clone
> [3] https://doc.rust-lang.org/std/clone/trait.Clone.html
>
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
>  bindings/rust/libgpiod/src/lib.rs         |  1 +
>  bindings/rust/libgpiod/src/line_info.rs   | 16 ++++++++++
>  bindings/rust/libgpiod/tests/line_info.rs | 53 +++++++++++++++++++++++++++++++
>  3 files changed, 70 insertions(+)

[...]

> diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
> index 32c4bb2..fe01a14 100644
> --- a/bindings/rust/libgpiod/src/line_info.rs
> +++ b/bindings/rust/libgpiod/src/line_info.rs
> @@ -58,6 +58,22 @@ impl Info {
>          self as *const _ as *mut _
>      }
>  
> +    /// Clones the [gpiod::gpiod_line_info] instance to an [InfoOwned]
> +    pub fn try_clone(&self) -> Result<InfoOwned> {

Hm... I realized that we have `event_clone()` for cloning an `Event`
and `settings_clone()` for cloning `line::Settings`. Should better
stay consistent here...

However, I think the name `try_clone()` sounds more suitable to me. Any
opinions? I could send a patch to rename the existing cloning methods
to `try_clone()`.

- Erik

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Erik Schilling]

> +/// Line info
> +///
> +/// This is the owned counterpart to [Info]. Due to a [Deref] implementation,
> +/// all functions of [Info] can also be called on this type.
> +#[derive(Debug)]
> +pub struct InfoOwned {
> +    info: *mut gpiod::gpiod_line_info,
> +}

While going through all the structs in order to add missing `Send`
implementations, it occured to me that it may be a bit confusing if
only this one type has the `Owned` suffix, while the others are also
"owned" but do not carry that suffix.

Not really sure how to resolve this... We could rename the non-owned
`Info` to something like `InfoRef` and turn `InfoOwned` back into
`Info`, but reading `&InfoRef` may be a bit weird?

Alternatively, we could rename all other structs to add the suffix...
Then, "Owned" would maybe sound confusing - given that no un-owned
variant exists.
Maybe "Box" would be a more suitable suffix in that case - borrowing
from the Box type name [1]?

Any opinions here?

[1] https://doc.rust-lang.org/std/boxed/struct.Box.html

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Viresh Kumar]

On 28-09-23, 14:27, Erik Schilling wrote:
> On Thu Sep 28, 2023 at 1:27 PM CEST, Viresh Kumar wrote:
> > > -    /// Get the Line info object associated with an event.
> > > -    pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> > > -        Info::new_internal(info, true)
> > > +    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> > > +        self as *const _ as *mut _
> >
> > What's wrong with keeping `_info` as `info` in the structure and using it
> > directly instead of this, since this is private anyway ?

Ahh, I missed that it is not *mut anymore. Shouldn't we mark it with & as well,
since it is a reference to the gpiod structure ? Something like ? (I must admit
that I have forgotten a lot of Rust during my long absence from work :)).

    _info: &'a gpiod::gpiod_line_info,


> We would still need to cast it the same way. One _could_ write:
> 
>     fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
>         &self.info as *const _ as *mut _
>     }

Can we use deref to just do this magically for us in this file somehow ?

> But the cast dance is still required since we need a *mut, but start
> with a readonly reference.
> 
> This is required since libgpiod's C lib keeps the struct internals
> opaque and does not make guarantees about immutable datastructures for
> any API calls.
> 
> Technically, the 1:1 mapping of this to Rust would be to restrict the
> entire API to `&mut self`. One could do that - it would probably allow
> us to advertise the structs as `Sync` - but it would require consumers
> to declare all libgpiod-related variables as `mut`.

-- 
viresh

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Viresh Kumar]

On 28-09-23, 15:24, Erik Schilling wrote:
> > +/// Line info
> > +///
> > +/// This is the owned counterpart to [Info]. Due to a [Deref] implementation,
> > +/// all functions of [Info] can also be called on this type.
> > +#[derive(Debug)]
> > +pub struct InfoOwned {
> > +    info: *mut gpiod::gpiod_line_info,
> > +}
> 
> While going through all the structs in order to add missing `Send`
> implementations, it occured to me that it may be a bit confusing if
> only this one type has the `Owned` suffix, while the others are also
> "owned" but do not carry that suffix.
> 
> Not really sure how to resolve this... We could rename the non-owned
> `Info` to something like `InfoRef` and turn `InfoOwned` back into
> `Info`, but reading `&InfoRef` may be a bit weird?

I like this one and none of the others.

> Alternatively, we could rename all other structs to add the suffix...
> Then, "Owned" would maybe sound confusing - given that no un-owned
> variant exists.
> Maybe "Box" would be a more suitable suffix in that case - borrowing
> from the Box type name [1]?
> 
> Any opinions here?
> 
> [1] https://doc.rust-lang.org/std/boxed/struct.Box.html

-- 
viresh

Re: [libgpiod][PATCH 2/3] bindings: rust: allow cloning line::Info -> line::OwnedInfo

[Viresh Kumar]

$subject: s/OwnedInfo/InfoOwned/

On 28-09-23, 14:52, Erik Schilling wrote:
> On Wed Sep 27, 2023 at 6:29 PM CEST, Erik Schilling wrote:
> > diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
> > index 32c4bb2..fe01a14 100644
> > --- a/bindings/rust/libgpiod/src/line_info.rs
> > +++ b/bindings/rust/libgpiod/src/line_info.rs
> > @@ -58,6 +58,22 @@ impl Info {
> >          self as *const _ as *mut _
> >      }
> >  
> > +    /// Clones the [gpiod::gpiod_line_info] instance to an [InfoOwned]
> > +    pub fn try_clone(&self) -> Result<InfoOwned> {
> 
> Hm... I realized that we have `event_clone()` for cloning an `Event`
> and `settings_clone()` for cloning `line::Settings`. Should better
> stay consistent here...
> 
> However, I think the name `try_clone()` sounds more suitable to me. Any
> opinions? I could send a patch to rename the existing cloning methods
> to `try_clone()`.

IIRC, I did try to use clone() and try_clone() earlier for something and there
were prototype issues, as they weren't matching with the standard library and so
had to innovate `event_clone()` and `settings_clone()`. `try_clone()` is anyday
better.

-- 
viresh

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Manos Pitsidianakis]

On Thu, 28 Sep 2023 16:24, Erik Schilling <erik.schilling@linaro.org> wrote:
>> +/// Line info
>> +///
>> +/// This is the owned counterpart to [Info]. Due to a [Deref] implementation,
>> +/// all functions of [Info] can also be called on this type.
>> +#[derive(Debug)]
>> +pub struct InfoOwned {
>> +    info: *mut gpiod::gpiod_line_info,
>> +}
>
>While going through all the structs in order to add missing `Send`
>implementations, it occured to me that it may be a bit confusing if
>only this one type has the `Owned` suffix, while the others are also
>"owned" but do not carry that suffix.
>
>Not really sure how to resolve this... We could rename the non-owned
>`Info` to something like `InfoRef` and turn `InfoOwned` back into
>`Info`, but reading `&InfoRef` may be a bit weird?

I think this sounds better.

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Erik Schilling]

On Fri Sep 29, 2023 at 12:39 PM CEST, Viresh Kumar wrote:
> On 28-09-23, 14:27, Erik Schilling wrote:
> > On Thu Sep 28, 2023 at 1:27 PM CEST, Viresh Kumar wrote:
> > > > -    /// Get the Line info object associated with an event.
> > > > -    pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> > > > -        Info::new_internal(info, true)
> > > > +    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> > > > +        self as *const _ as *mut _
> > >
> > > What's wrong with keeping `_info` as `info` in the structure and using it
> > > directly instead of this, since this is private anyway ?
>
> Ahh, I missed that it is not *mut anymore. Shouldn't we mark it with & as well,
> since it is a reference to the gpiod structure ? Something like ? (I must admit
> that I have forgotten a lot of Rust during my long absence from work :)).
>
>     _info: &'a gpiod::gpiod_line_info,

Technically, yes. But that would require a 'a lifetime parameter on
the `Info` struct. Then, instead of using `&Info` you would need to
use `Info<'a>` everywhere.

Which then gets ugly pretty fast since you need to carry it through all
impl blocks, the `Deref` implementation on `InfoOwned` and force it onto
the consumer of the lib.

I think staying with `&Info` keeps the API a lot simpler (and this code
simpler).

>
> > We would still need to cast it the same way. One _could_ write:
> > 
> >     fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> >         &self.info as *const _ as *mut _
> >     }
>
> Can we use deref to just do this magically for us in this file somehow ?

Hm... Not exactly sure what you mean here. Do you mean a `Deref`
implementation? That one would leak this implementation detail into
public API.

>
> > But the cast dance is still required since we need a *mut, but start
> > with a readonly reference.
> > 
> > This is required since libgpiod's C lib keeps the struct internals
> > opaque and does not make guarantees about immutable datastructures for
> > any API calls.
> > 
> > Technically, the 1:1 mapping of this to Rust would be to restrict the
> > entire API to `&mut self`. One could do that - it would probably allow
> > us to advertise the structs as `Sync` - but it would require consumers
> > to declare all libgpiod-related variables as `mut`.

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Viresh Kumar]

On 29-09-23, 12:58, Erik Schilling wrote:
> I think staying with `&Info` keeps the API a lot simpler (and this code
> simpler).

Right.

> >
> > > We would still need to cast it the same way. One _could_ write:
> > > 
> > >     fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> > >         &self.info as *const _ as *mut _
> > >     }
> >
> > Can we use deref to just do this magically for us in this file somehow ?
> 
> Hm... Not exactly sure what you mean here. Do you mean a `Deref`
> implementation? That one would leak this implementation detail into
> public API.

I was thinking of something else, not worth it. This is fine.

-- 
viresh

Re: [libgpiod][PATCH 2/3] bindings: rust: allow cloning line::Info -> line::OwnedInfo

[Erik Schilling]

On Fri Sep 29, 2023 at 12:50 PM CEST, Viresh Kumar wrote:
> $subject: s/OwnedInfo/InfoOwned/

Whoops. Flipped that around at some point. Forgot to fix here... Will do
once we agreed on a naming scheme :)

>
> On 28-09-23, 14:52, Erik Schilling wrote:
> > On Wed Sep 27, 2023 at 6:29 PM CEST, Erik Schilling wrote:
> > > diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
> > > index 32c4bb2..fe01a14 100644
> > > --- a/bindings/rust/libgpiod/src/line_info.rs
> > > +++ b/bindings/rust/libgpiod/src/line_info.rs
> > > @@ -58,6 +58,22 @@ impl Info {
> > >          self as *const _ as *mut _
> > >      }
> > >  
> > > +    /// Clones the [gpiod::gpiod_line_info] instance to an [InfoOwned]
> > > +    pub fn try_clone(&self) -> Result<InfoOwned> {
> > 
> > Hm... I realized that we have `event_clone()` for cloning an `Event`
> > and `settings_clone()` for cloning `line::Settings`. Should better
> > stay consistent here...
> > 
> > However, I think the name `try_clone()` sounds more suitable to me. Any
> > opinions? I could send a patch to rename the existing cloning methods
> > to `try_clone()`.
>
> IIRC, I did try to use clone() and try_clone() earlier for something and there
> were prototype issues, as they weren't matching with the standard library and so
> had to innovate `event_clone()` and `settings_clone()`. `try_clone()` is anyday
> better.

ACK. I am not aware of any trait like `TryClone`, but yeah: `Clone` and
`ToOwned` do not work for the reason outlined in the commit message.

I will then add a commit to rename the other `*_clone` functions to
`try_clone` in v2.

- Erik

Re: [libgpiod][PATCH 1/3] bindings: rust: fix soundness of line_info modeling

[Erik Schilling]

On Fri Sep 29, 2023 at 12:39 PM CEST, Viresh Kumar wrote:
> On 28-09-23, 15:24, Erik Schilling wrote:
> > > +/// Line info
> > > +///
> > > +/// This is the owned counterpart to [Info]. Due to a [Deref] implementation,
> > > +/// all functions of [Info] can also be called on this type.
> > > +#[derive(Debug)]
> > > +pub struct InfoOwned {
> > > +    info: *mut gpiod::gpiod_line_info,
> > > +}
> > 
> > While going through all the structs in order to add missing `Send`
> > implementations, it occured to me that it may be a bit confusing if
> > only this one type has the `Owned` suffix, while the others are also
> > "owned" but do not carry that suffix.
> > 
> > Not really sure how to resolve this... We could rename the non-owned
> > `Info` to something like `InfoRef` and turn `InfoOwned` back into
> > `Info`, but reading `&InfoRef` may be a bit weird?
>
> I like this one and none of the others.

OK :). With Manos also agreeing, I will do this then.

>
> > Alternatively, we could rename all other structs to add the suffix...
> > Then, "Owned" would maybe sound confusing - given that no un-owned
> > variant exists.
> > Maybe "Box" would be a more suitable suffix in that case - borrowing
> > from the Box type name [1]?
> > 
> > Any opinions here?
> > 
> > [1] https://doc.rust-lang.org/std/boxed/struct.Box.html

Re: [libgpiod][PATCH 3/3] bindings: rust: bump major for libgpiod crate

[Bartosz Golaszewski]

On Wed, Sep 27, 2023 at 6:30 PM Erik Schilling
<erik.schilling@linaro.org> wrote:
>
> Since the type changes around ownership of line_info instances are not
> necessarily transparent to the user, we bump the major for the next
> release of the crate.
>
> Note:
> I am using the term "major" as defined in the Rust SemVer compatibility
> guide [1], where the first non-zero digit is considered as "major".
>
> [1] https://doc.rust-lang.org/cargo/reference/semver.html
>
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
>  bindings/rust/libgpiod/Cargo.toml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bindings/rust/libgpiod/Cargo.toml b/bindings/rust/libgpiod/Cargo.toml
> index 518e5e5..3be4aa0 100644
> --- a/bindings/rust/libgpiod/Cargo.toml
> +++ b/bindings/rust/libgpiod/Cargo.toml
> @@ -4,7 +4,7 @@
>
>  [package]
>  name = "libgpiod"
> -version = "0.1.0"
> +version = "0.2.0"
>  authors = ["Viresh Kumar <viresh.kumar@linaro.org>"]
>  description = "libgpiod wrappers"
>  repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"
>
> --
> 2.41.0
>

Isn't it something that we should do right before tagging the release
once we know the final requirement for the version change? At least
this is what I do for the rest of libgpiod.

Bart

Re: [libgpiod][PATCH 3/3] bindings: rust: bump major for libgpiod crate

[Erik Schilling]

On Fri Sep 29, 2023 at 2:43 PM CEST, Bartosz Golaszewski wrote:
> On Wed, Sep 27, 2023 at 6:30 PM Erik Schilling
> <erik.schilling@linaro.org> wrote:
> >
> > Since the type changes around ownership of line_info instances are not
> > necessarily transparent to the user, we bump the major for the next
> > release of the crate.
> >
> > Note:
> > I am using the term "major" as defined in the Rust SemVer compatibility
> > guide [1], where the first non-zero digit is considered as "major".
> >
> > [1] https://doc.rust-lang.org/cargo/reference/semver.html
> >
> > Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> > ---
> >  bindings/rust/libgpiod/Cargo.toml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/bindings/rust/libgpiod/Cargo.toml b/bindings/rust/libgpiod/Cargo.toml
> > index 518e5e5..3be4aa0 100644
> > --- a/bindings/rust/libgpiod/Cargo.toml
> > +++ b/bindings/rust/libgpiod/Cargo.toml
> > @@ -4,7 +4,7 @@
> >
> >  [package]
> >  name = "libgpiod"
> > -version = "0.1.0"
> > +version = "0.2.0"
> >  authors = ["Viresh Kumar <viresh.kumar@linaro.org>"]
> >  description = "libgpiod wrappers"
> >  repository = "https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git"
> >
> > --
> > 2.41.0
> >
>
> Isn't it something that we should do right before tagging the release
> once we know the final requirement for the version change? At least
> this is what I do for the rest of libgpiod.

Yep. Probably thats better. Then we can also use that as a signal of
intended publish to crates.io.

Will drop from this series and send again once all current Rust patches
landed.

- Erik