Re: [PATCH V2 2/4] libgpiod: Add rust wrappers

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 16-12-21, 14:59, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> General comment on the naming convention:
> 
> The line from one of the examples: 'use libgpiod::chip::GpiodChip;'
> looks like it has a lot of redundancy in it. How about calling the
> crate gpiod and dropping the chip package entirely? Basically follow
> what C++ and python bindings do by having `use gpiod::Chip` etc.?

That would mean I move back all the code to lib.rs, which makes it contain too
much code. I am okay though if you want it that way.

> > +
> > +impl GpiodChipInternal {
> > +    /// Find a GPIO chip by path.
> > +    pub(crate) fn open(path: &str) -> Result<Self> {
> > +        let chip = unsafe { bindings::gpiod_chip_open(path.as_ptr() as *const c_char) };
> > +        if chip.is_null() {
> > +            return Err(Error::OperationFailed("GpiodChip open", IoError::last()));
> 
> One of my concerns last time was error handling. Does this now
> translate the error codes from the underlying C code to some kind of
> rust errors? Can you elaborate on how that works?

You can think of it like wrapping around or concatenation of errors in a way.
The above line will return this now:

Err(libgpiod::Error::OperationFailed("GpiodChip open", IoError::last())) and
will print like for EINVAL:

Operation GpiodChip open Failed: 22

The user crate can further wrap it around with their own error enums, but this
error information wouldn't be lost and won't be subject to interpretation as
well, as we return the real underlying value now. This is the right/best way to
do it I suppose in Rust, this is what I was suggested to do for the vhost-device
Rust crate I merged in rust-vmm project.

> I see it always returns IoError.

This returns the underling errno value.

> In my WIP C++ and Python code I try to translate the
> errnos into some meaningful exceptions - for instance EINVAL ->
> ::std::invalid_argument etc. Can we have a similar thing here?

I would avoid translating errors into some interpretation here and rather pass
on the real error number returned by kernel. That can be more useful in
debugging the failures later.

> > +    /// Get the GPIO chip name as represented in the kernel.
> > +    pub fn get_name(&self) -> Result<&str> {
> > +        // SAFETY: The string returned by libgpiod is guaranteed to live as long
> > +        // as the `struct GpiodChip`.
> > +        let name = unsafe { bindings::gpiod_chip_get_name(self.ichip.chip()) };
> > +        if name.is_null() {
> 
> This is not possible, the string can be empty at the very least but
> never null. Same for label and path.

Miguel, what's your take on stuff like this ? I am not sure if we should just
drop this check altogether.

> > +
> > +        match ret {
> > +            -1 => Err(Error::OperationFailed(
> > +                "GpiodChip info-event-wait",
> > +                IoError::last(),
> > +            )),
> > +            0 => Err(Error::OperationTimedOut),
> 
> Does it have to be an Error? It's pretty normal for an operation to
> time out, no?

Right, but it is still a timeout and should be represented like that. And it is
up for the caller to act based on it. For example in the gpiomon example, we
check if the returned value is OperationTimedOut or not, and we retry if it is.

> > +            _ => Ok(()),
> > +        }
> > +    }
> > +
> > +    /// Read a single line status change event from this chip. If no events are
> > +    /// pending, this function will block.
> > +    pub fn info_event_read(&self) -> Result<GpiodInfoEvent> {
> > +        GpiodInfoEvent::new(&self.ichip.clone())
> 
> Would you mind explaining what the clone() function does here to ichip
> and why it's needed.

Normally clone() is like copying of the objects, but in this case ichip is
declared as Arc (Atomic Reference Counting).

Invoking clone on Arc produces a new Arc instance, which points to the same
allocation on the heap as the source Arc, while increasing a reference count.

In rust, when the last reference of an object goes away, it is freed
automatically.

> > +impl GpiodEdgeEvent {
> > +    /// Get an event stored in the buffer.
> > +    pub fn new(buffer: &GpiodEdgeEventBuffer, index: u64) -> Result<Self> {
> > +        let event = unsafe { bindings::gpiod_edge_event_buffer_get_event(buffer.buffer(), index) };
> > +        if event.is_null() {
> > +            return Err(Error::OperationFailed(
> > +                "GpiodEdgeEvent buffer-get-event",
> > +                IoError::last(),
> > +            ));
> > +        }
> > +
> > +        Ok(Self { event })
> > +    }
> > +
> > +    /// Get the event type.
> > +    pub fn get_event_type(&self) -> Result<EdgeEvent> {
> > +        EdgeEvent::new(unsafe { bindings::gpiod_edge_event_get_event_type(self.event) } as u32)
> > +    }
> > +
> > +    /// Get the timestamp of the event.
> > +    pub fn get_timestamp(&self) -> Duration {
> > +        Duration::from_nanos(unsafe { bindings::gpiod_edge_event_get_timestamp(self.event) })
> > +    }
> > +
> > +    /// Get the offset of the line on which the event was triggered.
> > +    pub fn get_line_offset(&self) -> u32 {
> > +        unsafe { bindings::gpiod_edge_event_get_line_offset(self.event) }
> > +    }
> > +
> > +    /// Get the global sequence number of this event.
> > +    ///
> > +    /// Returns sequence number of the event relative to all lines in the
> > +    /// associated line request.
> > +    pub fn get_global_seqno(&self) -> u64 {
> > +        unsafe { bindings::gpiod_edge_event_get_global_seqno(self.event) }
> > +    }
> > +
> > +    /// Get the event sequence number specific to concerned line.
> > +    ///
> > +    /// Returns sequence number of the event relative to this line within the
> > +    /// lifetime of the associated line request.
> > +    pub fn get_line_seqno(&self) -> u64 {
> > +        unsafe { bindings::gpiod_edge_event_get_line_seqno(self.event) }
> > +    }
> > +}
> 
> AFAICT this object will not survive the parent buffer. Take a look at
> the current development version of the C++ bindings where I play with
> copy constructors to ensure that.

Adding back some of the removed code as well.

+impl Drop for GpiodEdgeEvent {
+    /// Free the edge event object and release all associated resources.
+    fn drop(&mut self) {
+        unsafe { bindings::gpiod_edge_event_free(self.event) }
+    }
+}

While testing this I found a bug few days back and I wonder why
gpiod_edge_event_free() even exists. The memory for "events" is allocated with
the buffer and gpiod_edge_event_free() shouldn't try to free a part of that.
This looks buggy.

Yes I realize that the edge event shouldn't exist past the buffer itself, I will
try to fix it in a Rusty way (maybe with Arc or something else).

-- 
viresh