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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

-- 
viresh