diff mbox series

[libgpiod,v2,3/3] bindings: rust: allow cloning line::InfoRef -> line::Info

Message ID 20230929-rust-line-info-soundness-v2-3-9782b7f20f26@linaro.org
State Superseded
Headers show
Series bindings: rust: fix modeling of line_info lifetimes | expand

Commit Message

Erik Schilling Sept. 29, 2023, 1:18 p.m. UTC
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(+)

Comments

Viresh Kumar Oct. 3, 2023, 9:01 a.m. UTC | #1
On 29-09-23, 15:18, 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/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 e3ea5e1..c9dd379 100644
> --- a/bindings/rust/libgpiod/src/line_info.rs
> +++ b/bindings/rust/libgpiod/src/line_info.rs
> @@ -58,6 +58,22 @@ impl InfoRef {
>          self as *const _ as *mut _
>      }
>  
> +    /// Clones the line info object.
> +    pub fn try_clone(&self) -> Result<Info> {
> +        // 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 { Info::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");
> +                });
> +            });
> +        }
>      }
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
diff mbox series

Patch

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 e3ea5e1..c9dd379 100644
--- a/bindings/rust/libgpiod/src/line_info.rs
+++ b/bindings/rust/libgpiod/src/line_info.rs
@@ -58,6 +58,22 @@  impl InfoRef {
         self as *const _ as *mut _
     }
 
+    /// Clones the line info object.
+    pub fn try_clone(&self) -> Result<Info> {
+        // 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 { Info::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");
+                });
+            });
+        }
     }
 }