[libgpiod][PATCH v2 0/5] core: provide information about the parent chip in line requests

[libgpiod][PATCH v2 0/5] core: provide information about the parent chip in line requests

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

While working on the DBus API, it occurred to me that while we can obtain
the list of requested offsets from a line request, this information lacks
context if we cannot get any information about the parent chip on which
the request was made.

We cannot reference the chip in any way as its lifetime is disconnected
from the request but we can at least provide the name of the chip used
to create the request providing some context for the offsets.

This series adds a new getter for struct gpiod_line_request and wrappers
for it for all bindings. This will be used in the upcoming DBus GPIO
manager code.

v1 -> v2:
- provide the chip name, not its filesystem path
- fix UNSAFE comments in rust bindings

Bartosz Golaszewski (5):
  core: provide gpiod_line_request_get_chip_name()
  tests: add a test-case for gpiod_line_request_get_chip_name()
  bindings: cxx: provide line_request::chip_name()
  bindings: python: provide the chip_name property in line_request
  bindings: rust: provide LineRequest::chip_name()

 bindings/cxx/gpiodcxx/line-request.hpp       |  6 ++++++
 bindings/cxx/line-request.cpp                | 10 +++++++++-
 bindings/cxx/tests/tests-line-request.cpp    |  6 ++++--
 bindings/python/gpiod/chip.py                |  1 +
 bindings/python/gpiod/ext/request.c          | 11 ++++++++++
 bindings/python/gpiod/line_request.py        | 12 +++++++++--
 bindings/python/tests/tests_line_request.py  | 13 +++++++-----
 bindings/rust/libgpiod/src/line_request.rs   | 16 +++++++++++++++
 bindings/rust/libgpiod/tests/line_request.rs | 14 +++++++++++++
 include/gpiod.h                              |  9 +++++++++
 lib/chip.c                                   |  7 ++++++-
 lib/internal.h                               |  3 ++-
 lib/line-request.c                           | 20 ++++++++++++++++++-
 tests/tests-line-request.c                   | 21 ++++++++++++++++++++
 14 files changed, 136 insertions(+), 13 deletions(-)

-- 
2.39.2

[libgpiod][PATCH v2 1/5] core: provide gpiod_line_request_get_chip_name()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

While we can get the list of requested offsets from a line-request object,
this information lacks context if we don't provide any data about the GPIO
chip the request was made on. Add a helper allowing users to get the name
of the parent chip.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/gpiod.h    |  9 +++++++++
 lib/chip.c         |  7 ++++++-
 lib/internal.h     |  3 ++-
 lib/line-request.c | 20 +++++++++++++++++++-
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/include/gpiod.h b/include/gpiod.h
index 3c13783..71ae798 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -1007,6 +1007,15 @@ gpiod_request_config_get_event_buffer_size(struct gpiod_request_config *config);
  */
 void gpiod_line_request_release(struct gpiod_line_request *request);
 
+/**
+ * @brief Get the name of the chip this request was made on.
+ * @param request Line request object.
+ * @return Name the GPIO chip device. The returned pointer is valid for the
+ * lifetime of the request object and must not be freed by the caller.
+ */
+const char *
+gpiod_line_request_get_chip_name(struct gpiod_line_request *request);
+
 /**
  * @brief Get the number of lines in the request.
  * @param request Line request object.
diff --git a/lib/chip.c b/lib/chip.c
index 7d4d21e..7c05e53 100644
--- a/lib/chip.c
+++ b/lib/chip.c
@@ -215,6 +215,7 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,
 {
 	struct gpio_v2_line_request uapi_req;
 	struct gpiod_line_request *request;
+	struct gpiochip_info info;
 	int ret;
 
 	assert(chip);
@@ -233,11 +234,15 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,
 	if (ret)
 		return NULL;
 
+	ret = read_chip_info(chip->fd, &info);
+	if (ret < 0)
+		return NULL;
+
 	ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req);
 	if (ret < 0)
 		return NULL;
 
-	request = gpiod_line_request_from_uapi(&uapi_req);
+	request = gpiod_line_request_from_uapi(&uapi_req, info.name);
 	if (!request) {
 		close(uapi_req.fd);
 		return NULL;
diff --git a/lib/internal.h b/lib/internal.h
index ef9b17e..61d7aec 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -26,7 +26,8 @@ void gpiod_request_config_to_uapi(struct gpiod_request_config *config,
 int gpiod_line_config_to_uapi(struct gpiod_line_config *config,
 			      struct gpio_v2_line_request *uapi_cfg);
 struct gpiod_line_request *
-gpiod_line_request_from_uapi(struct gpio_v2_line_request *uapi_req);
+gpiod_line_request_from_uapi(struct gpio_v2_line_request *uapi_req,
+			     const char *chip_name);
 int gpiod_edge_event_buffer_read_fd(int fd,
 				    struct gpiod_edge_event_buffer *buffer,
 				    size_t max_events);
diff --git a/lib/line-request.c b/lib/line-request.c
index e536355..e867d91 100644
--- a/lib/line-request.c
+++ b/lib/line-request.c
@@ -13,13 +13,15 @@
 #include "internal.h"
 
 struct gpiod_line_request {
+	char *chip_name;
 	unsigned int offsets[GPIO_V2_LINES_MAX];
 	size_t num_lines;
 	int fd;
 };
 
 struct gpiod_line_request *
-gpiod_line_request_from_uapi(struct gpio_v2_line_request *uapi_req)
+gpiod_line_request_from_uapi(struct gpio_v2_line_request *uapi_req,
+			     const char *chip_name)
 {
 	struct gpiod_line_request *request;
 
@@ -28,6 +30,13 @@ gpiod_line_request_from_uapi(struct gpio_v2_line_request *uapi_req)
 		return NULL;
 
 	memset(request, 0, sizeof(*request));
+
+	request->chip_name = strdup(chip_name);
+	if (!request->chip_name) {
+		free(request);
+		return NULL;
+	}
+
 	request->fd = uapi_req->fd;
 	request->num_lines = uapi_req->num_lines;
 	memcpy(request->offsets, uapi_req->offsets,
@@ -42,9 +51,18 @@ GPIOD_API void gpiod_line_request_release(struct gpiod_line_request *request)
 		return;
 
 	close(request->fd);
+	free(request->chip_name);
 	free(request);
 }
 
+GPIOD_API const char *
+gpiod_line_request_get_chip_name(struct gpiod_line_request *request)
+{
+	assert(request);
+
+	return request->chip_name;
+}
+
 GPIOD_API size_t
 gpiod_line_request_get_num_requested_lines(struct gpiod_line_request *request)
 {
-- 
2.39.2

[libgpiod][PATCH v2 2/5] tests: add a test-case for gpiod_line_request_get_chip_name()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Add a test case for the chip name getter on line-request objects.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tests/tests-line-request.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tests/tests-line-request.c b/tests/tests-line-request.c
index 0b985e2..1a58275 100644
--- a/tests/tests-line-request.c
+++ b/tests/tests-line-request.c
@@ -675,3 +675,24 @@ GPIOD_TEST_CASE(get_requested_offsets_less_and_more)
 	g_assert_cmpuint(retrieved[2], ==, 2);
 	g_assert_cmpuint(retrieved[3], ==, 3);
 }
+
+GPIOD_TEST_CASE(get_chip_name)
+{
+	static const guint offset = 4;
+
+	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 8, NULL);
+	g_autoptr(struct_gpiod_chip) chip = NULL;
+	g_autoptr(struct_gpiod_line_config) line_cfg = NULL;
+	g_autoptr(struct_gpiod_line_request) request = NULL;
+
+	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+	line_cfg = gpiod_test_create_line_config_or_fail();
+
+	gpiod_test_line_config_add_line_settings_or_fail(line_cfg, &offset, 1,
+							 NULL);
+
+	request = gpiod_test_chip_request_lines_or_fail(chip, NULL, line_cfg);
+
+	g_assert_cmpstr(g_gpiosim_chip_get_name(sim), ==,
+			gpiod_line_request_get_chip_name(request));
+}
-- 
2.39.2

[libgpiod][PATCH v2 3/5] bindings: cxx: provide line_request::chip_name()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Provide a wrapper around gpiod_line_request_get_chip_name() for C++
bindings and update the tests.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 bindings/cxx/gpiodcxx/line-request.hpp    |  6 ++++++
 bindings/cxx/line-request.cpp             | 10 +++++++++-
 bindings/cxx/tests/tests-line-request.cpp |  6 ++++--
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/bindings/cxx/gpiodcxx/line-request.hpp b/bindings/cxx/gpiodcxx/line-request.hpp
index c1e1520..8c1b474 100644
--- a/bindings/cxx/gpiodcxx/line-request.hpp
+++ b/bindings/cxx/gpiodcxx/line-request.hpp
@@ -75,6 +75,12 @@ public:
 	 */
 	void release();
 
+	/**
+	 * @brief Get the name of the chip this request was made on.
+	 * @return Name to the GPIO chip.
+	 */
+	::std::string chip_name() const;
+
 	/**
 	 * @brief Get the number of requested lines.
 	 * @return Number of lines in this request.
diff --git a/bindings/cxx/line-request.cpp b/bindings/cxx/line-request.cpp
index b0723c3..e8e0b96 100644
--- a/bindings/cxx/line-request.cpp
+++ b/bindings/cxx/line-request.cpp
@@ -63,6 +63,13 @@ GPIOD_CXX_API void line_request::release()
 	this->_m_priv->request.reset();
 }
 
+GPIOD_CXX_API ::std::string line_request::chip_name() const
+{
+	this->_m_priv->throw_if_released();
+
+	return ::gpiod_line_request_get_chip_name(this->_m_priv->request.get());
+}
+
 GPIOD_CXX_API ::std::size_t line_request::num_lines() const
 {
 	this->_m_priv->throw_if_released();
@@ -222,7 +229,8 @@ GPIOD_CXX_API ::std::ostream& operator<<(::std::ostream& out, const line_request
 	if (!request)
 		out << "gpiod::line_request(released)";
 	else
-		out << "gpiod::line_request(num_lines=" << request.num_lines() <<
+		out << "gpiod::line_request(chip=\"" << request.chip_name() <<
+		       "\", num_lines=" << request.num_lines() <<
 		       ", line_offsets=" << request.offsets() <<
 		       ", fd=" << request.fd() <<
 		       ")";
diff --git a/bindings/cxx/tests/tests-line-request.cpp b/bindings/cxx/tests/tests-line-request.cpp
index d1a56ae..9632ae0 100644
--- a/bindings/cxx/tests/tests-line-request.cpp
+++ b/bindings/cxx/tests/tests-line-request.cpp
@@ -468,14 +468,16 @@ TEST_CASE("line_request stream insertion operator works", "[line-request]")
 		.set_num_lines(4)
 		.build();
 
-	auto request = ::gpiod::chip(sim.dev_path())
+	auto chip = ::gpiod::chip(sim.dev_path());
+	auto request = chip
 		.prepare_request()
 		.add_line_settings({ 3, 1, 0, 2}, ::gpiod::line_settings())
 		.do_request();
 
 	::std::stringstream buf, expected;
 
-	expected << "gpiod::line_request(num_lines=4, line_offsets=gpiod::offsets(3, 1, 0, 2), fd=" <<
+	expected << "gpiod::line_request(chip=\"" << sim.name() <<
+		    "\", num_lines=4, line_offsets=gpiod::offsets(3, 1, 0, 2), fd=" <<
 		    request.fd() << ")";
 
 	SECTION("active request")
-- 
2.39.2

[libgpiod][PATCH v2 4/5] bindings: python: provide the chip_name property in line_request

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Provide a wrapper around gpiod_line_request_get_chip_name() for Python
bindings and update the tests.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 bindings/python/gpiod/chip.py               |  1 +
 bindings/python/gpiod/ext/request.c         | 11 +++++++++++
 bindings/python/gpiod/line_request.py       | 12 ++++++++++--
 bindings/python/tests/tests_line_request.py | 13 ++++++++-----
 4 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
index da93370..b3d8e61 100644
--- a/bindings/python/gpiod/chip.py
+++ b/bindings/python/gpiod/chip.py
@@ -315,6 +315,7 @@ class Chip:
         req_internal = self._chip.request_lines(line_cfg, consumer, event_buffer_size)
         request = LineRequest(req_internal)
 
+        request._chip_name = req_internal.chip_name
         request._offsets = req_internal.offsets
         request._name_map = name_map
         request._offset_map = offset_map
diff --git a/bindings/python/gpiod/ext/request.c b/bindings/python/gpiod/ext/request.c
index a32ff8f..5db69fe 100644
--- a/bindings/python/gpiod/ext/request.c
+++ b/bindings/python/gpiod/ext/request.c
@@ -37,6 +37,13 @@ static void request_finalize(request_object *self)
 		gpiod_edge_event_buffer_free(self->buffer);
 }
 
+static PyObject *
+request_chip_name(request_object *self, void *Py_UNUSED(ignored))
+{
+	return PyUnicode_FromString(
+			gpiod_line_request_get_chip_name(self->request));
+}
+
 static PyObject *
 request_num_lines(request_object *self, void *Py_UNUSED(ignored))
 {
@@ -92,6 +99,10 @@ static PyObject *request_fd(request_object *self, void *Py_UNUSED(ignored))
 }
 
 static PyGetSetDef request_getset[] = {
+	{
+		.name = "chip_name",
+		.get = (getter)request_chip_name,
+	},
 	{
 		.name = "num_lines",
 		.get = (getter)request_num_lines,
diff --git a/bindings/python/gpiod/line_request.py b/bindings/python/gpiod/line_request.py
index 096bf18..cde298f 100644
--- a/bindings/python/gpiod/line_request.py
+++ b/bindings/python/gpiod/line_request.py
@@ -212,10 +212,18 @@ class LineRequest:
         if not self._req:
             return "<LineRequest RELEASED>"
 
-        return "<LineRequest num_lines={} offsets={} fd={}>".format(
-            self.num_lines, self.offsets, self.fd
+        return '<LineRequest chip="{}" num_lines={} offsets={} fd={}>'.format(
+            self.chip_name, self.num_lines, self.offsets, self.fd
         )
 
+    @property
+    def chip_name(self) -> str:
+        """
+        Name of the chip this request was made on.
+        """
+        self._check_released()
+        return self._chip_name
+
     @property
     def num_lines(self) -> int:
         """
diff --git a/bindings/python/tests/tests_line_request.py b/bindings/python/tests/tests_line_request.py
index aa84b9a..f99b93d 100644
--- a/bindings/python/tests/tests_line_request.py
+++ b/bindings/python/tests/tests_line_request.py
@@ -529,11 +529,14 @@ class LineRequestStringRepresentation(TestCase):
         del self.sim
 
     def test_str(self):
-        with gpiod.request_lines(self.sim.dev_path, config={(2, 6, 4, 1): None}) as req:
-            self.assertEqual(
-                str(req),
-                "<LineRequest num_lines=4 offsets=[2, 6, 4, 1] fd={}>".format(req.fd),
-            )
+        with gpiod.Chip(self.sim.dev_path) as chip:
+            with chip.request_lines(config={(2, 6, 4, 1): None}) as req:
+                self.assertEqual(
+                    str(req),
+                    '<LineRequest chip="{}" num_lines=4 offsets=[2, 6, 4, 1] fd={}>'.format(
+                        self.sim.name, req.fd
+                    ),
+                )
 
     def test_str_released(self):
         req = gpiod.request_lines(self.sim.dev_path, config={(2, 6, 4, 1): None})
-- 
2.39.2

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

[Bartosz Golaszewski]

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)
+    }
+
     /// 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;
-- 
2.39.2

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

[Erik Schilling]

On Thu Jul 20, 2023 at 4:47 PM CEST, 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(+)

Reviewed-by: Erik Schilling <erik.schilling@linaro.org>

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

[Kent Gibson]

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.

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

[Kent Gibson]

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.

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

[Viresh Kumar]

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>

-- 
viresh

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

[Bartosz Golaszewski]

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.

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

[Kent Gibson]

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.

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

[Bartosz Golaszewski]

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