Re: [PATCH V4 1/8] libgpiod: Add libgpiod-sys rust crate

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 27-07-22, 10:57, Kent Gibson wrote:
> On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote:
> > +fn main() {
> > +    let files = vec![
> > +        "../../../lib/chip.c",
> > +        "../../../lib/chip-info.c",
> > +        "../../../lib/edge-event.c",
> > +        "../../../lib/info-event.c",
> > +        "../../../lib/internal.c",
> > +        "../../../lib/line-config.c",
> > +        "../../../lib/line-info.c",
> > +        "../../../lib/line-request.c",
> > +        "../../../lib/misc.c",
> > +        "../../../lib/request-config.c",
> > +    ];
> > +
> > +    #[cfg(feature = "generate")]
> > +    generate_bindings(&files);
> > +    build_gpiod(files);
> > +}
> 
> Shouldn't bindings wrap libgpiod and dynamically link against it rather
> than building and linking statically?

There are few problems I faced, because of which I had to do it this way.

- I couldn't find a way to do a "Make" for libgpiod from here and then link to
  the resultant library.

- libgpiod may not be automatically installed in the environment where the end
  user of these Rust APIs exists. So I had to build it.

- And then the API is changing a lot, maybe down the line once it is stable
  enough we can change this to something else.

> > diff --git a/bindings/rust/libgpiod-sys/src/lib.rs b/bindings/rust/libgpiod-sys/src/lib.rs
> > new file mode 100644
> > index 000000000000..3384863a567c
> > --- /dev/null
> > +++ b/bindings/rust/libgpiod-sys/src/lib.rs
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#[allow(
> > +    clippy::all,
> > +    deref_nullptr,
> > +    dead_code,
> > +    non_camel_case_types,
> > +    non_upper_case_globals,
> > +    non_snake_case,
> > +    improper_ctypes
> > +)]
> > +
> 
> Are all these really necessary?

Actually not, thanks for pointing this out.

> Builds mostly clean for me with just:
> 
>  +    non_camel_case_types,
>  +    non_upper_case_globals,
> 
> Both non_snake_case and deref_nullptr are only required for tests.

and if you want to run sanity checks with "fmt" or "clippy".

> The deref_nullptr masks several warnings like this:
> 
> warning: dereferencing a null pointer
>    --> src/bindings.rs:121:14
>     |
> 121 |             &(*(::std::ptr::null::<max_align_t>())).__clang_max_align_nonce1 as *const _ as usize
>     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed
>     |
>     = note: `#[warn(deref_nullptr)]` on by default
> 
> which is code generated by bindgen, which is a bit of a worry.
> It is only used for alignment tests, but you'd think they would disable
> the warning just around that code themselves.
> 
> Disabling deref_nullptr globally for all builds is at best poor form.

Even with this these will get disabled only for the code present in libgpiod-sys
crate, file bindgen.rs (the automatically generated one). This won't cause the
warnings to be skipped for the libgpiod rust wrappers in the libgpiod crate.

> Perhaps only disable it for test builds, i.e.
> 
> #[cfg_attr(test, allow(deref_nullptr, non_snake_case))]

I also run following normally:

cargo fmt --all -- --check; cargo clippy --workspace --bins --examples --benches --all-features -- -D warnings

to do sanity checks, etc. And this will also generate warnings, not just tests.

> > +mod bindings_raw {
> > +    #[cfg(feature = "generate")]
> > +    include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
> > +
> > +    #[cfg(not(feature = "generate"))]
> > +    include!("bindings.rs");
> > +}
> > +pub use bindings_raw::*;
> > diff --git a/bindings/rust/libgpiod-sys/wrapper.h b/bindings/rust/libgpiod-sys/wrapper.h
> > new file mode 100644
> > index 000000000000..7bc1158b7d90
> > --- /dev/null
> > +++ b/bindings/rust/libgpiod-sys/wrapper.h
> > @@ -0,0 +1,2 @@
> > +#include <string.h>
> > +#include "../../../include/gpiod.h"
> 
> The string.h is just to provide strlen() for the wrapper crate??
> (but also pulls in the other string functions)
> The wrapper crate already depends on libc - why not use libc::strlen()
> there and drop this include here?

Right, done.

> And then wrapper.h becomes redundant - call bindgen on gpiod.h directly.

Rust documentation specifically suggests wrapper.h to be created [1], maybe it
it is better to just keep it, even if we have a single entry in there.

-- 
viresh

[1] https://rust-lang.github.io/rust-bindgen/tutorial-2.html