| Message ID | 20230929-rust-line-info-soundness-v2-0-9782b7f20f26@linaro.org |
|---|---|
| Headers | show |
| Series | bindings: rust: fix modeling of line_info lifetimes | expand |
On 29-09-23, 15:18, Erik Schilling wrote: > diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs > index 81e1be6..9ef8f22 100644 > --- a/bindings/rust/libgpiod/src/chip.rs > +++ b/bindings/rust/libgpiod/src/chip.rs > @@ -107,7 +107,9 @@ 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 Info instance. > + Ok(unsafe { line::Info::from_raw_owned(info) }) Hmm, I was expecting the naming to be simplified in this version here. Now: Info::from_raw_owned() InfoRef::from_raw_non_owning() What I am suggesting: Info::from_raw() InfoRef::from_raw() Or maybe just `new()` routines for both ? I think structure names tell us enough about ownership here and we don't need to add it to functions.
On 29-09-23, 15:18, Erik Schilling wrote: > What is getting cloned is already clear from the type. This also aligns > a bit better with similar methods from the `std` crate [1]. > > [1] https://doc.rust-lang.org/std/index.html?search=try_clone > > Link: https://lore.kernel.org/r/CVUKC1HXG1P8.13XIUCCXN95F0@ablu-work > Signed-off-by: Erik Schilling <erik.schilling@linaro.org> > --- > bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs | 2 +- > bindings/rust/libgpiod/src/edge_event.rs | 3 ++- > bindings/rust/libgpiod/src/line_settings.rs | 4 ++-- > bindings/rust/libgpiod/tests/line_request.rs | 2 +- > 4 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs > index ad90d7b..8dbb496 100644 > --- a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs > +++ b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs > @@ -34,7 +34,7 @@ fn main() -> libgpiod::Result<()> { > let event = events.next().unwrap()?; > > // This will out live `event` and the next read_edge_events(). > - let cloned_event = libgpiod::request::Event::event_clone(event)?; > + let cloned_event = libgpiod::request::Event::try_clone(event)?; > > let events = request.read_edge_events(&mut buffer)?; > for event in events { > diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs > index 0c0cfbc..4c940ba 100644 > --- a/bindings/rust/libgpiod/src/edge_event.rs > +++ b/bindings/rust/libgpiod/src/edge_event.rs > @@ -25,7 +25,8 @@ use super::{ > pub struct Event(*mut gpiod::gpiod_edge_event); > > impl Event { > - pub fn event_clone(event: &Event) -> Result<Event> { > + /// Makes a copy of the event object. > + pub fn try_clone(event: &Event) -> Result<Event> { > // SAFETY: `gpiod_edge_event` is guaranteed to be valid here. > let event = unsafe { gpiod::gpiod_edge_event_copy(event.0) }; > if event.is_null() { > diff --git a/bindings/rust/libgpiod/src/line_settings.rs b/bindings/rust/libgpiod/src/line_settings.rs > index f0b3e9c..41b27e2 100644 > --- a/bindings/rust/libgpiod/src/line_settings.rs > +++ b/bindings/rust/libgpiod/src/line_settings.rs > @@ -52,8 +52,8 @@ impl Settings { > unsafe { gpiod::gpiod_line_settings_reset(self.settings) } > } > > - /// Makes copy of the settings object. > - pub fn settings_clone(&self) -> Result<Self> { > + /// Makes a copy of the settings object. > + pub fn try_clone(&self) -> Result<Self> { > // SAFETY: `gpiod_line_settings` is guaranteed to be valid here. > let settings = unsafe { gpiod::gpiod_line_settings_copy(self.settings) }; > if settings.is_null() { > diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs > index 9af5226..8731719 100644 > --- a/bindings/rust/libgpiod/tests/line_request.rs > +++ b/bindings/rust/libgpiod/tests/line_request.rs > @@ -272,7 +272,7 @@ mod line_request { > for offset in offsets { > lsettings.set_debounce_period(Duration::from_millis((100 + offset).into())); > lconfig > - .add_line_settings(&[offset as Offset], lsettings.settings_clone().unwrap()) > + .add_line_settings(&[offset as Offset], lsettings.try_clone().unwrap()) > .unwrap(); > } Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Tue Oct 3, 2023 at 10:58 AM CEST, Viresh Kumar wrote: > On 29-09-23, 15:18, Erik Schilling wrote: > > diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs > > index 81e1be6..9ef8f22 100644 > > --- a/bindings/rust/libgpiod/src/chip.rs > > +++ b/bindings/rust/libgpiod/src/chip.rs > > @@ -107,7 +107,9 @@ 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 Info instance. > > + Ok(unsafe { line::Info::from_raw_owned(info) }) > > Hmm, I was expecting the naming to be simplified in this version here. > > Now: > > Info::from_raw_owned() > InfoRef::from_raw_non_owning() > > What I am suggesting: > > Info::from_raw() > InfoRef::from_raw() > > Or maybe just `new()` routines for both ? > > I think structure names tell us enough about ownership here and we don't need to > add it to functions. Ah, I posted some weak opposition against that in [1], but failed to mention that again for v2. I mostly disliked `Info::from_raw()` being not very expressive without peeking at the type definition. But if you feel strongly about this, I am happy to change it. I would strongly vote for `from_raw()` since `new()` sounds like it would create a new instance, but really, we are just wrapping an already existing instance here. So... Shall I move to `from_raw()` or keep `from_raw_owned/non_owning()`? - Erik [1] https://lore.kernel.org/r/CVUJTBQZYN6B.17WXH28G8MKZ2@ablu-work
On 03-10-23, 11:17, Erik Schilling wrote: > Ah, I posted some weak opposition against that in [1], but failed to > mention that again for v2. I mostly disliked `Info::from_raw()` being > not very expressive without peeking at the type definition. But if you > feel strongly about this, I am happy to change it. I feel `Info` and `InfoRef` speak enough for themselves and what they contain and I don't see a need for the routines to be named specially anymore. > I would strongly vote for `from_raw()` since `new()` sounds like it > would create a new instance, but really, we are just wrapping an already > existing instance here. > > So... Shall I move to `from_raw()` or keep > `from_raw_owned/non_owning()`? I like the simplified version, i.e. `from_raw()` for sure. Unless someone else have an objection to that.
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 mind 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> Cc: Kent Gibson <warthog618@gmail.com> Signed-off-by: Erik Schilling <erik.schilling@linaro.org> --- Changes in v2: - Removed unneeded temporary variables - Added missing SAFETY comment - Renamed owning wrapper to `Event`, non-owning to `EventRef` - Renamed existing clone methods to try_clone() - Slightly tweaked try_clone() documentation - Dropped version bump commit - Added Fixes tag - CC'd Kent - suggested by vireshk since he reviewed his commits - Link to v1: https://lore.kernel.org/r/20230927-rust-line-info-soundness-v1-0-990dce6f18ab@linaro.org --- Erik Schilling (3): bindings: rust: fix soundness of line_info modeling bindings: rust: rename {event,settings}_clone to try_clone bindings: rust: allow cloning line::InfoRef -> line::Info .../libgpiod/examples/buffered_event_lifetimes.rs | 2 +- bindings/rust/libgpiod/src/chip.rs | 8 +- bindings/rust/libgpiod/src/edge_event.rs | 3 +- bindings/rust/libgpiod/src/info_event.rs | 8 +- bindings/rust/libgpiod/src/lib.rs | 1 + bindings/rust/libgpiod/src/line_info.rs | 140 +++++++++++++++------ bindings/rust/libgpiod/src/line_settings.rs | 4 +- bindings/rust/libgpiod/tests/line_info.rs | 53 ++++++++ bindings/rust/libgpiod/tests/line_request.rs | 2 +- 9 files changed, 173 insertions(+), 48 deletions(-) --- base-commit: ced90e79217793957b11414f47f8aa8a77c7a2d5 change-id: 20230927-rust-line-info-soundness-14c08e0d26e9 Best regards,