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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Jul 08, 2022 at 05:04:54PM +0530, Viresh Kumar wrote:
> This adds libgpiod-sys rust crate, which provides FFI (foreign function
> interface) bindings for libgpiod APIs.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

Just a quick qualifier before we get started - I'm relatively new to Rust
and this the first Rust code I've reviewed, so my opinions may not reflect
current idiomatic Rust or may even be complete rubbish.

> ---
>  .gitignore                            |  5 ++
>  bindings/rust/libgpiod-sys/Cargo.toml | 15 ++++++
>  bindings/rust/libgpiod-sys/build.rs   | 69 +++++++++++++++++++++++++++
>  bindings/rust/libgpiod-sys/src/lib.rs | 20 ++++++++
>  bindings/rust/libgpiod-sys/wrapper.h  |  2 +
>  5 files changed, 111 insertions(+)
>  create mode 100644 bindings/rust/libgpiod-sys/Cargo.toml
>  create mode 100644 bindings/rust/libgpiod-sys/build.rs
>  create mode 100644 bindings/rust/libgpiod-sys/src/lib.rs
>  create mode 100644 bindings/rust/libgpiod-sys/wrapper.h
> 
> diff --git a/.gitignore b/.gitignore
> index 58e1c5fc7e00..9541482d5efb 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -33,3 +33,8 @@ stamp-h1
>  # profiling
>  *.gcda
>  *.gcno
> +
> +# Added by cargo
> +
> +target
> +Cargo.lock
> diff --git a/bindings/rust/libgpiod-sys/Cargo.toml b/bindings/rust/libgpiod-sys/Cargo.toml
> new file mode 100644
> index 000000000000..77f82719d269
> --- /dev/null
> +++ b/bindings/rust/libgpiod-sys/Cargo.toml
> @@ -0,0 +1,15 @@
> +[package]
> +name = "libgpiod-sys"
> +version = "0.1.0"
> +edition = "2018"
> +
> +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
> +
> +[dependencies]
> +
> +[features]
> +generate = [ "bindgen" ]
> +
> +[build-dependencies]
> +bindgen = { version = "0.59.1", optional = true }
> +cc = "1.0.46"
> diff --git a/bindings/rust/libgpiod-sys/build.rs b/bindings/rust/libgpiod-sys/build.rs
> new file mode 100644
> index 000000000000..bbcd30f79d23
> --- /dev/null
> +++ b/bindings/rust/libgpiod-sys/build.rs
> @@ -0,0 +1,69 @@
> +#[cfg(feature = "generate")]
> +extern crate bindgen;
> +#[cfg(feature = "generate")]
> +use std::env;
> +#[cfg(feature = "generate")]
> +use std::path::PathBuf;
> +
> +#[cfg(feature = "generate")]
> +fn generate_bindings(files: &Vec<&str>) {
> +    // Tell cargo to invalidate the built crate whenever following files change
> +    println!("cargo:rerun-if-changed=wrapper.h");
> +
> +    for file in files {
> +        println!("cargo:rerun-if-changed={}", file);
> +    }
> +
> +    // The bindgen::Builder is the main entry point
> +    // to bindgen, and lets you build up options for
> +    // the resulting bindings.
> +    let bindings = bindgen::Builder::default()
> +        // The input header we would like to generate
> +        // bindings for.
> +        .header("wrapper.h")
> +        // Tell cargo to invalidate the built crate whenever any of the
> +        // included header files changed.
> +        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
> +        // Finish the builder and generate the bindings.
> +        .generate()
> +        // Unwrap the Result and panic on failure.
> +        .expect("Unable to generate bindings");
> +
> +    // Write the bindings to the $OUT_DIR/bindings.rs file.
> +    let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
> +    bindings
> +        .write_to_file(out_path.join("bindings.rs"))
> +        .expect("Couldn't write bindings!");
> +}
> +
> +fn build_gpiod(files: Vec<&str>) {
> +    // Tell Cargo that if the given file changes, to rerun this build script.
> +    println!("cargo:rerun-if-changed=../../../lib/");
> +
> +    // Use the `cc` crate to build a C file and statically link it.
> +    cc::Build::new()
> +        .files(files)
> +        .define("_GNU_SOURCE", None)
> +        .define("GPIOD_VERSION_STR", "\"libgpio-sys\"")
> +        .include("../../../include")
> +        .compile("gpiod");
> +}
> +
> +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?

> 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?
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.

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.
Perhaps only disable it for test builds, i.e.

#[cfg_attr(test, allow(deref_nullptr, non_snake_case))]

> +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?

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

Cheers,
Kent.