Re: [PATCH V7 5/8] libgpiod: Add gpiosim rust crate

[Kent Gibson <warthog618@xxxxxxxxx>]

On Fri, Oct 14, 2022 at 04:17:22PM +0530, Viresh Kumar wrote:
> This adds gpiosim rust crate, which provides helpers to emulate GPIO
> chips.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
>  bindings/rust/Cargo.toml         |   3 +-
>  bindings/rust/gpiosim/Cargo.toml |  18 ++
>  bindings/rust/gpiosim/build.rs   |  43 ++++
>  bindings/rust/gpiosim/src/lib.rs |  25 +++
>  bindings/rust/gpiosim/src/sim.rs | 323 +++++++++++++++++++++++++++++++
>  5 files changed, 411 insertions(+), 1 deletion(-)
>  create mode 100644 bindings/rust/gpiosim/Cargo.toml
>  create mode 100644 bindings/rust/gpiosim/build.rs
>  create mode 100644 bindings/rust/gpiosim/src/lib.rs
>  create mode 100644 bindings/rust/gpiosim/src/sim.rs
> 
> diff --git a/bindings/rust/Cargo.toml b/bindings/rust/Cargo.toml
> index 1e57ef2c0002..8721bc610b86 100644
> --- a/bindings/rust/Cargo.toml
> +++ b/bindings/rust/Cargo.toml
> @@ -2,5 +2,6 @@
>  
>  members = [
>      "libgpiod-sys",
> -    "libgpiod"
> +    "libgpiod",
> +    "gpiosim"
>  ]
> diff --git a/bindings/rust/gpiosim/Cargo.toml b/bindings/rust/gpiosim/Cargo.toml
> new file mode 100644
> index 000000000000..d6b4cc34339b
> --- /dev/null
> +++ b/bindings/rust/gpiosim/Cargo.toml
> @@ -0,0 +1,18 @@
> +[package]
> +name = "gpiosim"
> +version = "0.1.0"
> +edition = "2018"
> +

license and other keys...

> +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
> +
> +[dependencies]
> +libc = ">=0.2.39"
> +libgpiod = { path = "../libgpiod" }
> +vmm-sys-util = "=0.10.0"
> +
> +[features]
> +generate = [ "bindgen" ]
> +
> +[build-dependencies]
> +bindgen = { version = "0.59.1", optional = true }
> +cc = "1.0.46"
> diff --git a/bindings/rust/gpiosim/build.rs b/bindings/rust/gpiosim/build.rs
> new file mode 100644
> index 000000000000..460fb8c092c3
> --- /dev/null
> +++ b/bindings/rust/gpiosim/build.rs
> @@ -0,0 +1,43 @@
> +#[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() {
> +    // Tell cargo to invalidate the built crate whenever following files change
> +    println!("cargo:rerun-if-changed=../../../tests/gpiosim/gpiosim.h");
> +
> +    // 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("../../../tests/gpiosim/gpiosim.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 main() {
> +    #[cfg(feature = "generate")]
> +    generate_bindings();
> +
> +    println!("cargo:rustc-link-lib=kmod");
> +    println!("cargo:rustc-link-lib=mount");
> +    println!("cargo:rustc-link-search=./../../tests/gpiosim/.libs/");
> +    println!("cargo:rustc-link-lib=static=gpiosim");
> +}
> diff --git a/bindings/rust/gpiosim/src/lib.rs b/bindings/rust/gpiosim/src/lib.rs
> new file mode 100644
> index 000000000000..94d0ddb38e0f
> --- /dev/null
> +++ b/bindings/rust/gpiosim/src/lib.rs
> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0
> +

GPL-2.0.  Not so critical for the gpiosim, but still...

> +#[allow(non_camel_case_types, non_upper_case_globals)]
> +#[cfg_attr(test, allow(deref_nullptr, non_snake_case))]
> +#[allow(dead_code)]
> +mod bindings_raw {
> +    #[cfg(feature = "generate")]
> +    include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
> +
> +    #[cfg(not(feature = "generate"))]
> +    include!("bindings.rs");
> +}
> +
> +use bindings_raw::*;
> +
> +pub use bindings_raw::{
> +    GPIOSIM_HOG_DIR_INPUT, GPIOSIM_HOG_DIR_OUTPUT_HIGH, GPIOSIM_HOG_DIR_OUTPUT_LOW,
> +    GPIOSIM_PULL_DOWN, GPIOSIM_PULL_UP, GPIOSIM_VALUE_ACTIVE, GPIOSIM_VALUE_INACTIVE,
> +};

Create enums for pull, direction, and value and accept/return those rather
than passing around these consts as i32.

> +
> +#[allow(dead_code)]
> +mod sim;
> +

Handle the warning inside the module rather than the blanket allow.

Rename Sim.ctx to _ctx to clear the warning.

> +#[allow(unused_imports)]
> +pub use sim::*;

Which imports are unused and why?
I don't see any warnings after removing this allow.

> diff --git a/bindings/rust/gpiosim/src/sim.rs b/bindings/rust/gpiosim/src/sim.rs
> new file mode 100644
> index 000000000000..50977ea54ebb
> --- /dev/null
> +++ b/bindings/rust/gpiosim/src/sim.rs
> @@ -0,0 +1,323 @@
> +// SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
> +//
> +// Copyright 2022 Linaro Ltd. All Rights Reserved.
> +//     Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> +
> +use libc::strlen;
> +use std::os::raw::c_char;
> +use std::path::PathBuf;
> +use std::{slice, str};
> +
> +use vmm_sys_util::errno::Error as Errno;
> +
> +use libgpiod::{Error, Offset, OperationType, Result};
> +
> +use crate::*;
> +
> +/// Sim Ctx
> +#[derive(Debug)]
> +struct SimCtx {
> +    ctx: *mut gpiosim_ctx,
> +}
> +
> +unsafe impl Send for SimCtx {}
> +unsafe impl Sync for SimCtx {}
> +

Is gpiosim thread safe?

> +impl SimCtx {
> +    fn new() -> Result<Self> {
> +        let ctx = unsafe { gpiosim_ctx_new() };

SAFETY comments for all unsafe.

Cheers,
Kent.