[libgpiod v2][PATCH] misc: make gpiod_is_gpiochip_device() not set errno

[libgpiod v2][PATCH] misc: make gpiod_is_gpiochip_device() not set errno

[Bartosz Golaszewski]

This function should just report whether the file indicated by path is
a GPIO chip or not. Let's rework it to not set errno. Failure to open a
chip should still report errro numbers like before.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 lib/chip.c         |  2 +-
 lib/internal.c     | 65 +++++++++++++++++++++++++++++++++++++++++++++-
 lib/internal.h     |  2 ++
 lib/misc.c         | 62 ++-----------------------------------------
 tests/tests-misc.c |  2 ++
 5 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/lib/chip.c b/lib/chip.c
index fc3dda2..666a1ac 100644
--- a/lib/chip.c
+++ b/lib/chip.c
@@ -21,7 +21,7 @@ GPIOD_API struct gpiod_chip *gpiod_chip_open(const char *path)
 	struct gpiod_chip *chip;
 	int fd;
 
-	if (!gpiod_is_gpiochip_device(path))
+	if (!gpiod_check_gpiochip_device(path, true))
 		return NULL;
 
 	fd = open(path, O_RDWR | O_CLOEXEC | O_NONBLOCK);
diff --git a/lib/internal.c b/lib/internal.c
index b7da67e..ba7b90f 100644
--- a/lib/internal.c
+++ b/lib/internal.c
@@ -1,12 +1,75 @@
 // SPDX-License-Identifier: LGPL-2.1-or-later
-// SPDX-FileCopyrightText: 2021 Bartosz Golaszewski <brgl@bgdev.pl>
+// SPDX-FileCopyrightText: 2021-2022 Bartosz Golaszewski <brgl@bgdev.pl>
 
 #include <errno.h>
 #include <poll.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
+#include <sys/stat.h>
+#include <sys/sysmacros.h>
+#include <sys/types.h>
+#include <unistd.h>
 
 #include "internal.h"
 
+bool gpiod_check_gpiochip_device(const char *path, bool set_errno)
+{
+	char *realname, *sysfsp, devpath[64];
+	struct stat statbuf;
+	bool ret = false;
+	int rv;
+
+	rv = lstat(path, &statbuf);
+	if (rv)
+		goto out;
+
+	/*
+	 * Is it a symbolic link? We have to resolve it before checking
+	 * the rest.
+	 */
+	realname = S_ISLNK(statbuf.st_mode) ? realpath(path, NULL)
+					    : strdup(path);
+	if (realname == NULL)
+		goto out;
+
+	rv = stat(realname, &statbuf);
+	if (rv)
+		goto out_free_realname;
+
+	/* Is it a character device? */
+	if (!S_ISCHR(statbuf.st_mode)) {
+		errno = ENOTTY;
+		goto out_free_realname;
+	}
+
+	/* Is the device associated with the GPIO subsystem? */
+	snprintf(devpath, sizeof(devpath), "/sys/dev/char/%u:%u/subsystem",
+		 major(statbuf.st_rdev), minor(statbuf.st_rdev));
+
+	sysfsp = realpath(devpath, NULL);
+	if (!sysfsp)
+		goto out_free_realname;
+
+	if (strcmp(sysfsp, "/sys/bus/gpio") != 0) {
+		/* This is a character device but not the one we're after. */
+		errno = ENODEV;
+		goto out_free_sysfsp;
+	}
+
+	ret = true;
+
+out_free_sysfsp:
+	free(sysfsp);
+out_free_realname:
+	free(realname);
+out:
+	if (!set_errno)
+		errno = 0;
+	return ret;
+}
+
 int gpiod_poll_fd(int fd, uint64_t timeout_ns)
 {
 	struct timespec ts;
diff --git a/lib/internal.h b/lib/internal.h
index c87df91..12f184e 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -18,6 +18,8 @@
 
 #define GPIOD_BIT(nr)	(1UL << (nr))
 
+bool gpiod_check_gpiochip_device(const char *path, bool set_errno);
+
 struct gpiod_chip_info *
 gpiod_chip_info_from_uapi(struct gpiochip_info *uapi_info);
 struct gpiod_line_info *
diff --git a/lib/misc.c b/lib/misc.c
index 5c326eb..b0899b3 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -1,71 +1,13 @@
 // SPDX-License-Identifier: LGPL-2.1-or-later
-// SPDX-FileCopyrightText: 2017-2021 Bartosz Golaszewski <bartekgola@gmail.com>
+// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@gmail.com>
 
-#include <errno.h>
 #include <gpiod.h>
-#include <stdint.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/stat.h>
-#include <sys/sysmacros.h>
-#include <sys/types.h>
-#include <unistd.h>
 
 #include "internal.h"
 
 GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
 {
-	char *realname, *sysfsp, devpath[64];
-	struct stat statbuf;
-	bool ret = false;
-	int rv;
-
-	rv = lstat(path, &statbuf);
-	if (rv)
-		goto out;
-
-	/*
-	 * Is it a symbolic link? We have to resolve it before checking
-	 * the rest.
-	 */
-	realname = S_ISLNK(statbuf.st_mode) ? realpath(path, NULL)
-					    : strdup(path);
-	if (realname == NULL)
-		goto out;
-
-	rv = stat(realname, &statbuf);
-	if (rv)
-		goto out_free_realname;
-
-	/* Is it a character device? */
-	if (!S_ISCHR(statbuf.st_mode)) {
-		errno = ENOTTY;
-		goto out_free_realname;
-	}
-
-	/* Is the device associated with the GPIO subsystem? */
-	snprintf(devpath, sizeof(devpath), "/sys/dev/char/%u:%u/subsystem",
-		 major(statbuf.st_rdev), minor(statbuf.st_rdev));
-
-	sysfsp = realpath(devpath, NULL);
-	if (!sysfsp)
-		goto out_free_realname;
-
-	if (strcmp(sysfsp, "/sys/bus/gpio") != 0) {
-		/* This is a character device but not the one we're after. */
-		errno = ENODEV;
-		goto out_free_sysfsp;
-	}
-
-	ret = true;
-
-out_free_sysfsp:
-	free(sysfsp);
-out_free_realname:
-	free(realname);
-out:
-	return ret;
+	return gpiod_check_gpiochip_device(path, false);
 }
 
 GPIOD_API const char *gpiod_version_string(void)
diff --git a/tests/tests-misc.c b/tests/tests-misc.c
index c473aff..d3c9a82 100644
--- a/tests/tests-misc.c
+++ b/tests/tests-misc.c
@@ -15,7 +15,9 @@
 GPIOD_TEST_CASE(is_gpiochip_bad)
 {
 	g_assert_false(gpiod_is_gpiochip_device("/dev/null"));
+	g_assert_cmpint(errno, ==, 0);
 	g_assert_false(gpiod_is_gpiochip_device("/dev/nonexistent"));
+	g_assert_cmpint(errno, ==, 0);
 }
 
 GPIOD_TEST_CASE(is_gpiochip_good)
-- 
2.34.1

Re: [libgpiod v2][PATCH] misc: make gpiod_is_gpiochip_device() not set errno

[Andy Shevchenko]

On Fri, Jul 1, 2022 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> This function should just report whether the file indicated by path is
> a GPIO chip or not. Let's rework it to not set errno. Failure to open a
> chip should still report errro numbers like before.

errrrrrrr!

-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod v2][PATCH] misc: make gpiod_is_gpiochip_device() not set errno

[Kent Gibson]

On Fri, Jul 01, 2022 at 01:00:56PM +0200, Bartosz Golaszewski wrote:
> This function should just report whether the file indicated by path is
> a GPIO chip or not. Let's rework it to not set errno. Failure to open a
> chip should still report errro numbers like before.
> 

This is will break my tool patch, for sure.
My gpiodetect uses the errno behaviour to give a clue as to why a chip
might not be available to a user, and that work was already done in
gpiod_is_gpiochip_device().
There might be other places the errno was propagated as well, but
whatever, I'll sort something out.

Cheers,
Kent.

Re: [libgpiod v2][PATCH] misc: make gpiod_is_gpiochip_device() not set errno

[Bartosz Golaszewski]

On Fri, Jul 1, 2022 at 1:43 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Jul 01, 2022 at 01:00:56PM +0200, Bartosz Golaszewski wrote:
> > This function should just report whether the file indicated by path is
> > a GPIO chip or not. Let's rework it to not set errno. Failure to open a
> > chip should still report errro numbers like before.
> >
>
> This is will break my tool patch, for sure.
> My gpiodetect uses the errno behaviour to give a clue as to why a chip
> might not be available to a user, and that work was already done in
> gpiod_is_gpiochip_device().
> There might be other places the errno was propagated as well, but
> whatever, I'll sort something out.
>

Doesn't it make more sense to call gpiod_is_gpiochip_device() and then
if it returns true, just try to open it and then report failure?

Bart

Re: [libgpiod v2][PATCH] misc: make gpiod_is_gpiochip_device() not set errno

[Kent Gibson]

On Fri, Jul 01, 2022 at 01:50:46PM +0200, Bartosz Golaszewski wrote:
> On Fri, Jul 1, 2022 at 1:43 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Jul 01, 2022 at 01:00:56PM +0200, Bartosz Golaszewski wrote:
> > > This function should just report whether the file indicated by path is
> > > a GPIO chip or not. Let's rework it to not set errno. Failure to open a
> > > chip should still report errro numbers like before.
> > >
> >
> > This is will break my tool patch, for sure.
> > My gpiodetect uses the errno behaviour to give a clue as to why a chip
> > might not be available to a user, and that work was already done in
> > gpiod_is_gpiochip_device().
> > There might be other places the errno was propagated as well, but
> > whatever, I'll sort something out.
> >
> 
> Doesn't it make more sense to call gpiod_is_gpiochip_device() and then
> if it returns true, just try to open it and then report failure?
> 

Not for the case where the chip doesn't exist -
gpiod_is_gpiochip_device() has already worked that out.

But don't worry - I'll sort something out.

Cheers,
Kent.