| 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 |
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 --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"); + }); + }); + } } }
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(+)