Re: [PATCH V7 0/8] libgpiod: Add Rust bindings

[Björn Roy Baron <bjorn3_gh@xxxxxxxxxxxxxx>]

On Friday, October 21st, 2022 at 11:39, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:

> Hi Björn,
> 
> I have bounced (mutt's feature) the initial emails to your and other
> email ids that Miguel added. The patches should be in your inbox now.

Thanks! I receive the patches.

> 
> On 20-10-22, 13:29, Björn Roy Baron wrote:
> 
> > At
> > https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/lib.rs#L469
> > and elsewhere you might want to use `CStr::from_ptr(version)`. This
> > does the `strlen` call for you and you can convert it to an `&str`
> > using `.to_str()`.
> 
> > At
> > https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/chip.rs#L171
> > you could use `CString` and use the `.as_ptr()` method to get a
> > null-terminated string. Not sure if it would be nicer that what you
> > currently have though.
> 
> 
> These two were nice. Thanks.
> 
> > At
> > https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/edge_event.rs#L46
> > the lifetimes are unclear. Is Event allowed to outlive the buffer?
> > Can you add a lifetime annotation like fn event_clone<'a>(event:
> > &Event<'a>) -> Result<Event<'a>> if it isn't allowed to outlive the
> > buffer or fn event_clone<'a, 'b>(event: &Event<'a>) ->
> > Result<Event<'b>> if it is allowed to outlive the buffer. I'm not
> > sure which of the two the lifetime elision rules cause the current
> > code to be equivalent of, but even if it is correct, explicitly
> > stating the lifetime here is clearer IMHO.
> 
> 
> An Event created using Event::new() can't outlive the buffer, though
> an Event created using Event::event_clone() can.
> 
> I tried to play around it based on your suggestion and here is the
> diff, does it make sense ?
> 
> diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs
> index b36c23601bb4..0d328ebb2b03 100644
> --- a/bindings/rust/libgpiod/src/edge_event.rs
> +++ b/bindings/rust/libgpiod/src/edge_event.rs
> @@ -33,7 +33,7 @@ pub struct Event<'b> {
> 
> 
> impl<'b> Event<'b> {
> 
> /// Get an event stored in the buffer.
> - pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Self> {
> 
> + pub(crate) fn new(buffer: &'b Buffer, index: usize) -> Result<Event<'b>> {

This looks good to me.

> 
> // SAFETY: The `gpiod_edge_event` returned by libgpiod is guaranteed to live as long
> // as the `struct Event`.
> let event = unsafe {
> @@ -52,22 +52,6 @@ impl<'b> Event<'b> {
> 
> })
> }
> 
> - pub fn event_clone(event: &Event) -> Result<Self> {
> 
> - // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
> - let event = unsafe { gpiod::gpiod_edge_event_copy(event.event) };
> - if event.is_null() {
> - return Err(Error::OperationFailed(
> - OperationType::EdgeEventCopy,
> - Errno::last(),
> - ));
> - }
> -
> - Ok(Self {
> - buffer: None,
> - event,
> - })
> - }
> -
> /// Get the event type.
> pub fn event_type(&self) -> Result<EdgeKind> {
> 
> // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
> @@ -105,6 +89,27 @@ impl<'b> Event<'b> {
> 
> }
> }
> 
> +impl<'e, 'b> Event<'e> {
> 
> + pub fn event_clone(event: &Event<'b>) -> Result<Event<'e>>
> 
> + where
> + 'e: 'b,

Using `Event<'b>` on both sides should work fine. `Event` is covariant in it's lifetime parameter, so `Event<'b>` can be turned into `Event<'e>` with `'e` being a shorter lifetime than `'b`. What you wrote here is not incorrect, so if you prefer keeping it this way that is fine with me.

> + {
> + // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
> + let event = unsafe { gpiod::gpiod_edge_event_copy(event.event) };
> + if event.is_null() {
> + return Err(Error::OperationFailed(
> + OperationType::EdgeEventCopy,
> + Errno::last(),
> + ));
> + }
> +
> + Ok(Self {
> + buffer: None,
> + event,
> + })
> + }
> +}
> +
> 
> --
> viresh

Cheers,
Björn