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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Jul 20, 2023 at 7:04 AM Erik Schilling
<erik.schilling@xxxxxxxxxx> wrote:
>
> On Wed Jul 19, 2023 at 9:20 PM CEST, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > Provide a wrapper around gpiod_line_request_get_chip_path() for Rust
> > bindings and add a test-case.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > ---
> [...]
> > diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
> > index 1140aa9..2caab14 100644
> > --- a/bindings/rust/libgpiod/src/line_request.rs
> > +++ b/bindings/rust/libgpiod/src/line_request.rs
> [...]
> > @@ -25,6 +26,17 @@ impl Request {
> >          Ok(Self { request })
> >      }
> >
> > +    pub fn chip_path(&self) -> Result<&str> {
>
> A rustdoc comment `/// <explanation>` on the function may be helpful.
> The other functions have some (though those could probably also be a
> little longer...).
>
> More importantly, _if_ this function is returning a file path, then I
> would consider to return a Path [1]. The conversion from &str -> Path is
> "0-cost" and makes the API more explicit. `Sim::dev_path()` also returns
> a PathBuf so the conversion in the test would become a little easier.
>

I wanted to stay in line with chip's path() getter which also returns
Result<&str>. As you're saying - the user can convert it at 0 cost if
needed.

Bart

> > +        // SAFETY: The string returned by libgpiod is guaranteed to live as long
> > +        // as the `struct LineRequest`.
> > +        let path = unsafe { gpiod::gpiod_line_request_get_chip_path(self.request) };
>
> The SAFETY comment should explain why the following `unsafe` block is
> safe. For this block, the lifetime of the string does not matter for
> safety. Instead, it should explain why self.request is valid and safe
> to use.
>
> Maybe like this?
>
> +        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
> +        // as `&self`
>
>
> > +        // SAFETY: The string is guaranteed to be valid here by the C API.
> > +        unsafe { CStr::from_ptr(path) }
> > +            .to_str()
> > +            .map_err(Error::StringNotUtf8)
> > +    }
>
> Here the lifetime of the string is important then! Checking the
> Cstr::from_ptr doc [2], one needs to ensure that:
>
> - The memory pointed to by ptr must contain a valid nul terminator at
>   the end of the string.
> - ptr must be valid for reads of bytes up to and including the null
>   terminator.
> - The memory referenced by the returned CStr must not be mutated for the
>   duration of lifetime 'a.
>
> The SAFETY comment should explain why these three requirements are
> satisfied.
>
> Suggestion:
>
> +        // 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`.
>

I'll take the suggestions verbatim, thanks!

Bart

> [1] https://doc.rust-lang.org/stable/std/path/struct.Path.html
> [2] https://doc.rust-lang.org/std/ffi/struct.CStr.html#method.from_ptr
>
> [...]
>
> LGTM otherwise.
>
> - Erik