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

[Kent Gibson <warthog618@gmail.com>]

On Thu, Dec 16, 2021 at 02:59:25PM +0100, Bartosz Golaszewski wrote:
> On Thu, Dec 2, 2021 at 12:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Add rust wrappers around FFI bindings to provide a convenient interface
> > for the users.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> 
> Hi Viresh, thanks for the hard work.
> 
> Obviously this is not something we can merge yet but it's a good base
> to continue the work.
> 
> 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.?
> 

I'm of the opinion that a rust implementation would be better targetting
the ioctls directly rather than libgpiod, as my Go library does and for
essentially the same reasons.
To test that theory, and as an exercise to learn rust, I've been writing
one, and so far I've been calling it gpiod :-|.

Since this crate targets libgpiod it would make sense for it to
remain named libgpiod.

I agree with the chip::Chip repitition point - found the same thing
myself.  OTOH chip::Info and line::Info etc work well, and in rust
you can always rename them as you import them into your namespace if it
bothers you.  So overall I'm still on the fence on this one.

> [snip]
> 
> > +
> > +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? I see it always
> returns IoError. 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?
> 
> [snip]
> 
> > +
> > +    /// 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.
> 

On the subject of strings, are paths, names, labels, and consumers
guaranteed to be valid UTF-8?
If not then failing to convert them using UTF-8 is not an error (not
here but elsewhere in the patch).

Cheers,
Kent.