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

["Erik Schilling" <erik.schilling@xxxxxxxxxx>]

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.

> +        // 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`.

[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