diff mbox series

[libgpiod,v2,5/5] bindings: rust: provide LineRequest::chip_name()

Message ID 20230720144747.73276-6-brgl@bgdev.pl
State New
Headers show
Series core: provide information about the parent chip in line requests | expand

Commit Message

Bartosz Golaszewski July 20, 2023, 2:47 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
bindings and add a test-case.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
 bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
 2 files changed, 30 insertions(+)

Comments

Kent Gibson July 21, 2023, 3:15 a.m. UTC | #1 
On Thu, Jul 20, 2023 at 04:47:47PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> bindings and add a test-case.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
>  bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
> index 1140aa9..737c06f 100644
> --- a/bindings/rust/libgpiod/src/line_request.rs
> +++ b/bindings/rust/libgpiod/src/line_request.rs
> @@ -2,6 +2,7 @@
>  // SPDX-FileCopyrightText: 2022 Linaro Ltd.
>  // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
>  
> +use std::ffi::CStr;
>  use std::os::unix::prelude::AsRawFd;
>  use std::time::Duration;
>  
> @@ -25,6 +26,21 @@ impl Request {
>          Ok(Self { request })
>      }
>  
> +    /// Get the name of the chip this request was made on.
> +    pub fn chip_name(&self) -> Result<&str> {
> +        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
> +        // as `&self`
> +        let name = unsafe { gpiod::gpiod_line_request_get_chip_name(self.request) };
> +
> +        // 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`.
> +        unsafe { CStr::from_ptr(name) }
> +            .to_str()
> +            .map_err(Error::StringNotUtf8)
> +    }
> +

I would drop the name temp var myself, but that is just a nit.

Other than that the series looks good to me.

Reviewed-by: Kent Gibson <warthog618@gmail.com>

Cheers,
Kent.
Kent Gibson July 21, 2023, 3:27 a.m. UTC | #2 
On Thu, Jul 20, 2023 at 04:47:47PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> bindings and add a test-case.
> 

One other thing I noticed - just a heads up that the bindings generate a
new clippy warning from lastest nightly:
(rustc 1.73.0-nightly (399b06823 2023-07-20) (from rustc 1.73.0-nightly (ad963232d 2023-07-14))

warning: usage of an `Arc` that is not `Send` or `Sync`
  --> libgpiod/src/chip.rs:75:21
   |
75 |         let ichip = Arc::new(Internal::open(path)?);
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: the trait `Send` is not implemented for `Internal`
   = note: the trait `Sync` is not implemented for `Internal`
   = note: required for `Arc<Internal>` to implement `Send` and `Sync`
   = help: consider using an `Rc` instead or wrapping the inner type with a `Mutex`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#arc_with_non_send_sync
   = note: `#[warn(clippy::arc_with_non_send_sync)]` on by default

warning: `libgpiod` (lib) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 11.01s


That is independent of this patch.
And it is the nightly, which is a WIP so that may change - I noticed an
older nightly reported this as an error.

Cheers,
Kent.
Viresh Kumar July 21, 2023, 6:55 a.m. UTC | #3 
On 20-07-23, 16:47, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> bindings and add a test-case.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
>  bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
>  2 files changed, 30 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Bartosz Golaszewski July 21, 2023, 6:35 p.m. UTC | #4 
On Fri, Jul 21, 2023 at 5:15 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Jul 20, 2023 at 04:47:47PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> > bindings and add a test-case.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
> >  bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
> > index 1140aa9..737c06f 100644
> > --- a/bindings/rust/libgpiod/src/line_request.rs
> > +++ b/bindings/rust/libgpiod/src/line_request.rs
> > @@ -2,6 +2,7 @@
> >  // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> >  // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
> >
> > +use std::ffi::CStr;
> >  use std::os::unix::prelude::AsRawFd;
> >  use std::time::Duration;
> >
> > @@ -25,6 +26,21 @@ impl Request {
> >          Ok(Self { request })
> >      }
> >
> > +    /// Get the name of the chip this request was made on.
> > +    pub fn chip_name(&self) -> Result<&str> {
> > +        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
> > +        // as `&self`
> > +        let name = unsafe { gpiod::gpiod_line_request_get_chip_name(self.request) };
> > +
> > +        // 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`.
> > +        unsafe { CStr::from_ptr(name) }
> > +            .to_str()
> > +            .map_err(Error::StringNotUtf8)
> > +    }
> > +
>
> I would drop the name temp var myself, but that is just a nit.
>

I would too but rust was making it very difficult with borrow semantics. :)

Bart

> Other than that the series looks good to me.
>
> Reviewed-by: Kent Gibson <warthog618@gmail.com>
>
> Cheers,
> Kent.
Kent Gibson July 22, 2023, 2:07 a.m. UTC | #5 
On Fri, Jul 21, 2023 at 08:35:07PM +0200, Bartosz Golaszewski wrote:
> On Fri, Jul 21, 2023 at 5:15 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Jul 20, 2023 at 04:47:47PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> > > bindings and add a test-case.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >  bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
> > >  bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
> > >  2 files changed, 30 insertions(+)
> > >
> > > diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
> > > index 1140aa9..737c06f 100644
> > > --- a/bindings/rust/libgpiod/src/line_request.rs
> > > +++ b/bindings/rust/libgpiod/src/line_request.rs
> > > @@ -2,6 +2,7 @@
> > >  // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> > >  // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
> > >
> > > +use std::ffi::CStr;
> > >  use std::os::unix::prelude::AsRawFd;
> > >  use std::time::Duration;
> > >
> > > @@ -25,6 +26,21 @@ impl Request {
> > >          Ok(Self { request })
> > >      }
> > >
> > > +    /// Get the name of the chip this request was made on.
> > > +    pub fn chip_name(&self) -> Result<&str> {
> > > +        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
> > > +        // as `&self`
> > > +        let name = unsafe { gpiod::gpiod_line_request_get_chip_name(self.request) };
> > > +
> > > +        // 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`.
> > > +        unsafe { CStr::from_ptr(name) }
> > > +            .to_str()
> > > +            .map_err(Error::StringNotUtf8)
> > > +    }
> > > +
> >
> > I would drop the name temp var myself, but that is just a nit.
> >
> 
> I would too but rust was making it very difficult with borrow semantics. :)
> 

Really?  What error are you getting?

This works for me:

    /// Get the name of the chip this request was made on.
    pub fn chip_name(&self) -> Result<&str> {
        // 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`.
        unsafe { CStr::from_ptr(gpiod::gpiod_line_request_get_chip_name(self.request)) }
            .to_str()
            .map_err(Error::StringNotUtf8)
    }

And the last sentence of the SAFETY comment looks redundant to me -
it is just repeating what the signature already says.
(otherwise the return would be something like Result<&'a mut String>)

Cheers,
Kent.
Bartosz Golaszewski July 22, 2023, 12:30 p.m. UTC | #6 
On Sat, Jul 22, 2023 at 4:07 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jul 21, 2023 at 08:35:07PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Jul 21, 2023 at 5:15 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Jul 20, 2023 at 04:47:47PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Provide a wrapper around gpiod_line_request_get_chip_name() for Rust
> > > > bindings and add a test-case.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > ---
> > > >  bindings/rust/libgpiod/src/line_request.rs   | 16 ++++++++++++++++
> > > >  bindings/rust/libgpiod/tests/line_request.rs | 14 ++++++++++++++
> > > >  2 files changed, 30 insertions(+)
> > > >
> > > > diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
> > > > index 1140aa9..737c06f 100644
> > > > --- a/bindings/rust/libgpiod/src/line_request.rs
> > > > +++ b/bindings/rust/libgpiod/src/line_request.rs
> > > > @@ -2,6 +2,7 @@
> > > >  // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> > > >  // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
> > > >
> > > > +use std::ffi::CStr;
> > > >  use std::os::unix::prelude::AsRawFd;
> > > >  use std::time::Duration;
> > > >
> > > > @@ -25,6 +26,21 @@ impl Request {
> > > >          Ok(Self { request })
> > > >      }
> > > >
> > > > +    /// Get the name of the chip this request was made on.
> > > > +    pub fn chip_name(&self) -> Result<&str> {
> > > > +        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
> > > > +        // as `&self`
> > > > +        let name = unsafe { gpiod::gpiod_line_request_get_chip_name(self.request) };
> > > > +
> > > > +        // 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`.
> > > > +        unsafe { CStr::from_ptr(name) }
> > > > +            .to_str()
> > > > +            .map_err(Error::StringNotUtf8)
> > > > +    }
> > > > +
> > >
> > > I would drop the name temp var myself, but that is just a nit.
> > >
> >
> > I would too but rust was making it very difficult with borrow semantics. :)
> >
>
> Really?  What error are you getting?
>
> This works for me:
>
>     /// Get the name of the chip this request was made on.
>     pub fn chip_name(&self) -> Result<&str> {
>         // 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`.
>         unsafe { CStr::from_ptr(gpiod::gpiod_line_request_get_chip_name(self.request)) }
>             .to_str()
>             .map_err(Error::StringNotUtf8)
>     }
>
> And the last sentence of the SAFETY comment looks redundant to me -
> it is just repeating what the signature already says.
> (otherwise the return would be something like Result<&'a mut String>)
>
> Cheers,
> Kent.

I guess it does work. This looks less obvious to me though after all,
so I left the previous version. I removed the last part of the SAFETY
comment though as per your suggestion.

Thanks
Bart
diff mbox series

Patch

diff --git a/bindings/rust/libgpiod/src/line_request.rs b/bindings/rust/libgpiod/src/line_request.rs
index 1140aa9..737c06f 100644
--- a/bindings/rust/libgpiod/src/line_request.rs
+++ b/bindings/rust/libgpiod/src/line_request.rs
@@ -2,6 +2,7 @@ 
 // SPDX-FileCopyrightText: 2022 Linaro Ltd.
 // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
 
+use std::ffi::CStr;
 use std::os::unix::prelude::AsRawFd;
 use std::time::Duration;
 
@@ -25,6 +26,21 @@  impl Request {
         Ok(Self { request })
     }
 
+    /// Get the name of the chip this request was made on.
+    pub fn chip_name(&self) -> Result<&str> {
+        // SAFETY: The `gpiod_line_request` is guaranteed to be live as long
+        // as `&self`
+        let name = unsafe { gpiod::gpiod_line_request_get_chip_name(self.request) };
+
+        // 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`.
+        unsafe { CStr::from_ptr(name) }
+            .to_str()
+            .map_err(Error::StringNotUtf8)
+    }
+
     /// Get the number of lines in the request.
     pub fn num_lines(&self) -> usize {
         // SAFETY: `gpiod_line_request` is guaranteed to be valid here.
diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
index d49874f..9af5226 100644
--- a/bindings/rust/libgpiod/tests/line_request.rs
+++ b/bindings/rust/libgpiod/tests/line_request.rs
@@ -59,6 +59,20 @@  mod line_request {
     mod verify {
         use super::*;
 
+        #[test]
+        fn chip_name() {
+            const GPIO: Offset = 2;
+            let mut config = TestConfig::new(NGPIO).unwrap();
+            config.lconfig_add_settings(&[GPIO]);
+            config.request_lines().unwrap();
+
+            let arc = config.sim();
+            let sim = arc.lock().unwrap();
+            let chip_name = sim.chip_name().clone();
+
+            assert_eq!(config.request().chip_name().unwrap(), chip_name);
+        }
+
         #[test]
         fn custom_consumer() {
             const GPIO: Offset = 2;