| Message ID | 20230927-rust-line-info-soundness-v1-2-990dce6f18ab@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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
$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.
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"); + }); + }); + } } }
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(+)