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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 17-12-21, 05:52, Wedson Almeida Filho wrote:
> Some drive-by comments. I haven't reviewed the code in detail.
> 
> On Fri, 17 Dec 2021 at 05:01, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> >
> > 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.
> 
> You can keep things in different source files/modules and then expose
> them from lib.rs. For example, we have something like this in the
> kernel crate:

Right I did that kind of thing somewhere else too. This should work better.

> mod types;
> pub use crate::types::{bit, bits_iter, Mode, Opaque, ScopeGuard};
> 
> So these are implemented in `types.rs` but exposed directly from the
> kernel crate as `kernel::bit`, `kernel::bits_iter`, etc.
> 
> > > > +
> > > > +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) };
> 
> Note that Rust strings are not necessarily null-terminated, so if
> `gpiod_chip_open` is expecting a null-terminated string,

I guess it would, though my code works just fine right now and I am not sure
why, maybe by chance the next byte is zero or something :)

> this isn't
> always going to work. In the kernel we have null-terminated strings as
> a CStr type, and they can be initialised with constants using the
> `c_str!` macro. You'll probably need something similar here if you
> want to avoid allocations (otherwise you can just allocate a
> null-terminated copy of `path`, use it, then free it).

So I must do something like this then ?

    pub(crate) fn open(path: &str) -> Result<Self> {
        let chip = unsafe { bindings::gpiod_chip_open(CStr::from_ptr(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
> 
> Seems ok for printing. Is there a way to get the actual error code (22
> in the example above) from the error returned? (As opposed to just a
> string.)

The entire thing is returned to the caller,
i.e.  libgpiod::Error::OperationFailed("GpiodChip open", 22).

I think caller should be able to get the code out of it somehow, won't something
like this work ?

let errno = match error {
        libgpiod::Error::OperationFailed(msg, errno) => errno,
        _ => 0,
};

> > > > +    /// 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.
> 
> I'm obviously not Miguel :) But here's my take on this:

You are always welcome to provide feedback, I am the newbie here :)

> I think it's better to keep the check even if it's not possible today.
>
> But if you choose to remove it, then you probably want to note in the
> next SAFETY comment for `from_raw_parts` (which I think you forgot to
> add) that `name` is always non-null.

Right.

> > > > +    /// 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.
> 
> Your description of `Arc` is accurate, but it doesn't look like the
> code does what you intended. Note that you're passing the _address_ of
> the cloned arc to `GpiodInfoEvent::new`, so what you're doing here is:
> incrementing the refcount, calling GpiodInfoEvent::new, then
> immediately decrementing the count. If you want to keep the extra
> reference alive, you must transfer ownership of it (and hold on to it
> for as long as you want it alive).

Ahh, this instance contains a clone by mistake here. It was correctly done at
other places where I wanted the newly allocated structure to contain a reference
to underlying ichip. I have removed clone() from two places where I was passing
the reference instead.

-- 
viresh