Re: [libgpiod][PATCH v2 5/5] bindings: rust: provide LineRequest::chip_name()

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Jul 22, 2023 at 4:07 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Jul 21, 2023 at 08:35:07PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Jul 21, 2023 at 5:15 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> > >
> > > On Thu, Jul 20, 2023 at 04:47:47PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > >
> > > > Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> > > > bindings and add a test-case.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > > > ---
> > > >  bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
> > > >  bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
> > > >  2 files changed, 30 insertions(+)
> > > >
> > > > diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
> > > > index 1140aa9..737c06f 100644
> > > > --- a/bindings/rust/libgpiod/src/line_request.rs
> > > > +++ b/bindings/rust/libgpiod/src/line_request.rs
> > > > @@ -2,6 +2,7 @@
> > > >  // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> > > >  // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > > >
> > > > +use std::ffi::CStr;
> > > >  use std::os::unix::prelude::AsRawFd;
> > > >  use std::time::Duration;
> > > >
> > > > @@ -25,6 +26,21 @@ impl Request {
> > > >          Ok(Self { request })
> > > >      }
> > > >
> > > > +    /// Get the name of the chip this request was made on.
> > > > +    pub fn chip_name(&self) -> Result<&str> {
> > > > +        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
> > > > +        // as `&self`
> > > > +        let name = unsafe { gpiod::gpiod_line_request_get_chip_name(self.request) };
> > > > +
> > > > +        // SAFETY: The string is guaranteed to be valid, non-null and immutable
> > > > +        // by the C API for the lifetime of the `gpiod_line_request`. The
> > > > +        // `gpiod_line_request` is living as long as `&self`. The string is
> > > > +        // returned read-only with a lifetime of `&self`.
> > > > +        unsafe { CStr::from_ptr(name) }
> > > > +            .to_str()
> > > > +            .map_err(Error::StringNotUtf8)
> > > > +    }
> > > > +
> > >
> > > I would drop the name temp var myself, but that is just a nit.
> > >
> >
> > I would too but rust was making it very difficult with borrow semantics. :)
> >
>
> Really?  What error are you getting?
>
> This works for me:
>
>     /// Get the name of the chip this request was made on.
>     pub fn chip_name(&self) -> Result<&str> {
>         // SAFETY: The string is guaranteed to be valid, non-null and immutable
>         // by the C API for the lifetime of the `gpiod_line_request`. The
>         // `gpiod_line_request` is living as long as `&self`. The string is
>         // returned read-only with a lifetime of `&self`.
>         unsafe { CStr::from_ptr(gpiod::gpiod_line_request_get_chip_name(self.request)) }
>             .to_str()
>             .map_err(Error::StringNotUtf8)
>     }
>
> And the last sentence of the SAFETY comment looks redundant to me -
> it is just repeating what the signature already says.
> (otherwise the return would be something like Result<&'a mut String>)
>
> Cheers,
> Kent.

I guess it does work. This looks less obvious to me though after all,
so I left the previous version. I removed the last part of the SAFETY
comment though as per your suggestion.

Thanks
Bart