Re: [PATCH V6 3/8] libgpiod: Add rust wrapper crate

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Sep 26, 2022 at 1:08 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> Add rust wrapper crate, around the libpiod-sys crate added earlier, to
> provide a convenient interface for the users.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---

Hey Viresh!

Thanks for being patient with me. :)

I obviously cannot give a comprehensive review of Rust code as you
know it much better than I do but there's one thing that bothers me at
first glance. Please see below.

[snip]

> diff --git a/bindings/rust/libgpiod/src/event_buffer.rs b/bindings/rust/libgpiod/src/event_buffer.rs
> new file mode 100644
> index 000000000000..e272e7aa9e9d
> --- /dev/null
> +++ b/bindings/rust/libgpiod/src/event_buffer.rs
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
> +//
> +// Copyright 2022 Linaro Ltd. All Rights Reserved.
> +//     Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> +
> +use std::os::raw::c_ulong;
> +use std::sync::Arc;
> +
> +use vmm_sys_util::errno::Error as Errno;
> +
> +use super::{edge, gpiod, Error, OperationType, Result};
> +
> +/// Line edge events buffer
> +#[derive(Clone, Debug, Eq, PartialEq)]
> +pub(crate) struct BufferInternal {
> +    buffer: *mut gpiod::gpiod_edge_event_buffer,
> +}
> +
> +impl BufferInternal {
> +    /// Create a new edge event buffer.
> +    ///
> +    /// If capacity equals 0, it will be set to a default value of 64. If
> +    /// capacity is larger than 1024, it will be limited to 1024.
> +    pub fn new(capacity: usize) -> Result<Self> {
> +        let buffer = unsafe { gpiod::gpiod_edge_event_buffer_new(capacity as c_ulong) };
> +        if buffer.is_null() {
> +            return Err(Error::OperationFailed(
> +                OperationType::EdgeEventBufferNew,
> +                Errno::last(),
> +            ));
> +        }
> +
> +        Ok(Self { buffer })
> +    }
> +
> +    /// Private helper, Returns gpiod_edge_event_buffer
> +    pub(crate) fn buffer(&self) -> *mut gpiod::gpiod_edge_event_buffer {
> +        self.buffer
> +    }
> +}
> +
> +impl Drop for BufferInternal {
> +    /// Free the edge event buffer and release all associated resources.
> +    fn drop(&mut self) {
> +        unsafe { gpiod::gpiod_edge_event_buffer_free(self.buffer) };
> +    }
> +}
> +
> +/// Line edge events buffer
> +#[derive(Clone, Debug, Eq, PartialEq)]
> +pub struct Buffer {
> +    ibuffer: Arc<BufferInternal>,
> +}
> +
> +impl Buffer {
> +    /// Create a new edge event buffer.
> +    ///
> +    /// If capacity equals 0, it will be set to a default value of 64. If
> +    /// capacity is larger than 1024, it will be limited to 1024.
> +    pub fn new(capacity: usize) -> Result<Self> {
> +        Ok(Self {
> +            ibuffer: Arc::new(BufferInternal::new(capacity)?),
> +        })
> +    }
> +
> +    /// Private helper, Returns gpiod_edge_event_buffer
> +    pub(crate) fn buffer(&self) -> *mut gpiod::gpiod_edge_event_buffer {
> +        self.ibuffer.buffer()
> +    }
> +
> +    /// Get the capacity of the event buffer.
> +    pub fn capacity(&self) -> usize {
> +        unsafe { gpiod::gpiod_edge_event_buffer_get_capacity(self.buffer()) as usize }
> +    }
> +
> +    /// Read an event stored in the buffer.
> +    pub fn event(&self, index: u64) -> Result<edge::Event> {
> +        edge::Event::new(&self.ibuffer, index)
> +    }

In Event's new() you call gpiod_edge_event_buffer_get_event() which
returns a pointer to an event stored inside the buffer. There's also
the event_clone() function that calls gpiod_edge_event_copy() but I
don't see it called anywhere. Should users pay attention to the
lifetime of the buffer storing the event? Because IMO if the buffer
goes out of scope, the program will crash attempting to access the
event.

In C++ the events in the buffer can only be accessed as const
edge_event& so it's clear we're only holding a reference to an object
existing somewhere else. Internally, the object stored in the buffer
doesn't copy the edge event, only holds a C pointer to the event
structure in struct gpiod_edge_event_buffer. Only upon calling
edge_event& edge_event::operator=(const edge_event& other) will we
trigger a copy.

This way doing `for (const auto& event: buffer)` allows us to iterate
over events without doing any additional allocations.

Can we reproduce that behavior in Rust? For instance, the above
function could return a borrowed reference and then we could have some
interface to trigger the copy? Maybe do an implicit copy like in C++?
I don't know if that's possible though.

Bartosz

> +
> +    /// Get the number of events the buffer contains.
> +    pub fn len(&self) -> usize {
> +        unsafe { gpiod::gpiod_edge_event_buffer_get_num_events(self.buffer()) as usize }
> +    }
> +
> +    /// Check if buffer is empty.
> +    pub fn is_empty(&self) -> bool {
> +        self.len() == 0
> +    }
> +}

[snip]