Re: [libgpiod][PATCH] core: relax gpiod_is_gpiochip_device() even more

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Apr 1, 2021 at 12:17 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> Currently libgpiod requires that the GPIO chip character device be named
> 'gpiochip%u' in devfs. However it's a perfectly valid use-case to have
> the device file renamed by udev (or equivalent) to anything else.
>
> Modify gpiod_is_gpiochip_device() to check the major and minor device
> numbers first and then ensure that the device in question is associated
> with the GPIO subsystem. No longer check the name.

As long as it passes all tests and nicely handles symlinks, I'm
completely fine with it
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

P.S. /offtopic/ gentle reminder about my PR, can we proceed with it?

> Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> ---
>  lib/core.c | 41 ++++++++---------------------------------
>  1 file changed, 8 insertions(+), 33 deletions(-)
>
> diff --git a/lib/core.c b/lib/core.c
> index c1fb8ec..32c4238 100644
> --- a/lib/core.c
> +++ b/lib/core.c
> @@ -182,11 +182,10 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
>
>  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
>  {
> -       char *name, *realname, *sysfsp, sysfsdev[16], devstr[16];
> +       char *realname, *sysfsp, devpath[64];
>         struct stat statbuf;
>         bool ret = false;
> -       int rv, fd;
> -       ssize_t rd;
> +       int rv;
>
>         rv = lstat(path, &statbuf);
>         if (rv)
> @@ -217,15 +216,15 @@ GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
>                 goto out_free_realname;
>         }
>
> -       /* Get the basename. */
> -       name = basename(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));
>
> -       /* Do we have a corresponding sysfs attribute? */
> -       rv = asprintf(&sysfsp, "/sys/bus/gpio/devices/%s/dev", name);
> -       if (rv < 0)
> +       sysfsp = realpath(devpath, NULL);
> +       if (!sysfsp)
>                 goto out_free_realname;
>
> -       if (access(sysfsp, R_OK) != 0) {
> +       if (strcmp(sysfsp, "/sys/bus/gpio") != 0) {
>                 /*
>                  * This is a character device but not the one we're after.
>                  * Before the introduction of this function, we'd fail with
> @@ -237,30 +236,6 @@ GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
>                 goto out_free_sysfsp;
>         }
>
> -       /*
> -        * Make sure the major and minor numbers of the character device
> -        * correspond to the ones in the dev attribute in sysfs.
> -        */
> -       snprintf(devstr, sizeof(devstr), "%u:%u",
> -                major(statbuf.st_rdev), minor(statbuf.st_rdev));
> -
> -       fd = open(sysfsp, O_RDONLY);
> -       if (fd < 0)
> -               goto out_free_sysfsp;
> -
> -       memset(sysfsdev, 0, sizeof(sysfsdev));
> -       rd = read(fd, sysfsdev, sizeof(sysfsdev) - 1);
> -       close(fd);
> -       if (rd < 0)
> -               goto out_free_sysfsp;
> -
> -       rd--; /* Ignore trailing newline. */
> -       if ((size_t)rd != strlen(devstr) ||
> -           strncmp(sysfsdev, devstr, rd) != 0) {
> -               errno = ENODEV;
> -               goto out_free_sysfsp;
> -       }
> -
>         ret = true;
>
>  out_free_sysfsp:
> --
> 2.30.1
>


-- 
With Best Regards,
Andy Shevchenko