Re: [PATCH V7 0/8] libgpiod: Add Rust bindings

[Björn Roy Baron <bjorn3_gh@xxxxxxxxxxxxxx>]

On Friday, October 14th, 2022 at 19:03, Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote:

> On Fri, Oct 14, 2022 at 12:47 PM Viresh Kumar viresh.kumar@xxxxxxxxxx wrote:
> 
> > Here is another version of rust bindings for libgpiod v2.0, based of the
> > next/libgpiod-2.0.
> > 
> > Pushed here:
> > 
> > https://github.com/vireshk/libgpiod v7
> > 
> > [I have pushed v6 and v5 there too, in case someone wants to look at the
> > changes].

It looks pretty good to me. I do have a couple of minor suggestions though.

https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/Cargo.toml#L10 should use 0.2.39 without >= to ensure that publishing a 0.3 version of libc with breaking changes won't break the build. Cargo treats "0.2.39" as ">=0.2.39, <0.3".

At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/Cargo.toml#L13 the vmm-sys-utils dependency should be unpinned from "=0.10.0" to "0.10.0". Otherwise having any crate depend on a newer semver compatible version will cause a build error. While Cargo allows multiple semver incompatible versions of a crate, it doesn't allow multiple semver compatible versions as it wouldn't know which version to use for a crate that says it works with both versions.

At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/lib.rs#L469 and elsewhere you might want to use `CStr::from_ptr(version)`. This does the `strlen` call for you and you can convert it to an `&str` using `.to_str()`.

At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/chip.rs#L171 you could use `CString` and use the `.as_ptr()` method to get a null-terminated string. Not sure if it would be nicer that what you currently have though.

At https://github.com/vireshk/libgpiod/blob/3e7fb99173856a3995360fc3fad51220c4b4e90e/bindings/rust/libgpiod/src/edge_event.rs#L46 the lifetimes are unclear. Is Event allowed to outlive the buffer? Can you add a lifetime annotation like fn event_clone<'a>(event: &Event<'a>) -> Result<Event<'a>> if it isn't allowed to outlive the buffer or fn event_clone<'a, 'b>(event: &Event<'a>) -> Result<Event<'b>> if it is allowed to outlive the buffer. I'm not sure which of the two the lifetime elision rules cause the current code to be equivalent of, but even if it is correct, explicitly stating the lifetime here is clearer IMHO.

As for the question about test skipping elsewhere in this thread, rust has the #[ignore] attribute to ignore tests at compile time (with a command line flag to run ignored tests anyway), but nothing to ignore tests at runtime unfortunately.

Cheers,
Björn

nb: I can't reply to a mail I didn't receive directly right now due to mail provider limitations. I'm working on sorting this out, but for now I'm going to reply to the mail I did receive directly.