Re: [PATCH libgpiod-v2] tools: Restore support for opening chips by label

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Jul 28, 2021 at 11:19 PM Ben Hutchings <ben.hutchings@xxxxxxx> wrote:
>
> Support for opening chips by label was removed because labels
> are not necessarily unique and lookup by label requires opening
> every GPIO device.
>
> I don't think these concerns apply to the tools.  They will normally
> be run by root, and if a label is specified it's because it's known to
> be unique.
>
> This adds a chip_open_by_label() function to tools-common.c, which:
>
> 1. Scans the /dev/ directory for GPIO devices, and opens each in turn
> 2. Checks whether the label matches, and that the label isn't used by
>    two distinct devices
> 3. If all devices can be opened and exactly one matches the label,
>    return that device
>
> It changes chip_open_lookup() to call chip_open_by_label() as a final
> fallback, rather than the previous behaviour.  This should avoid
> producing spurious error messages if a tool is run by a user that can
> only access a subset of GPIO devices.
>
> Signed-off-by: Ben Hutchings <ben.hutchings@xxxxxxx>
> ---
>  tools/tools-common.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/tools/tools-common.c b/tools/tools-common.c
> index 36724d5..d2665e8 100644
> --- a/tools/tools-common.c
> +++ b/tools/tools-common.c
> @@ -4,6 +4,7 @@
>  /* Common code for GPIO tools. */
>
>  #include <ctype.h>
> +#include <dirent.h>
>  #include <errno.h>
>  #include <gpiod.h>
>  #include <libgen.h>
> @@ -146,6 +147,66 @@ static struct gpiod_chip *chip_open_by_number(unsigned int num)
>         return chip;
>  }
>
> +struct gpiod_chip *chip_open_by_label(const char *label)
> +{
> +       struct gpiod_chip *chip = NULL, *next = NULL;
> +       struct dirent **entries;
> +       int num_chips, i, error = 0;
> +
> +       num_chips = scandir("/dev/", &entries, chip_dir_filter, alphasort);
> +       if (num_chips < 0) {
> +               error = errno;
> +               fprintf(stderr, "unable to scan /dev: %s\n", strerror(error));
> +               goto out;
> +       }
> +
> +       for (i = 0; i < num_chips; i++) {
> +               next = chip_open_by_name(entries[i]->d_name);
> +               if (!next) {
> +                       error = errno;
> +                       fprintf(stderr, "unable to open %s: %s\n",
> +                               entries[i]->d_name, strerror(error));

How about using access() to check the permissions first? This way we
wouldn't need to spam the user with error messages - we'd just
silently ignore chips we don't have access to.

Bart

> +                       break;
> +               }
> +
> +               if (chip && !strcmp(gpiod_chip_get_name(chip),
> +                                   gpiod_chip_get_name(next))) {
> +                       /* chip and next are actually the same device */
> +                       gpiod_chip_close(next);
> +               } else if (!strcmp(gpiod_chip_get_label(next), label)) {
> +                       /* label matches; check it's not a duplicate */
> +                       if (chip) {
> +                               fprintf(stderr,
> +                                       "multiple chips have the label \"%s\"\n",
> +                                       label);
> +                               error = EINVAL;
> +                               break;
> +                       }
> +                       chip = next;
> +               } else {
> +                       gpiod_chip_close(next);
> +               }
> +       }
> +
> +       if (error) {
> +               if (chip)
> +                       gpiod_chip_close(chip);
> +               if (next)
> +                       gpiod_chip_close(next);
> +               chip = NULL;
> +       } else if (!chip) {
> +               error = ENOENT;
> +       }
> +
> +       for (i = 0; i < num_chips; i++)
> +               free(entries[i]);
> +       free(entries);
> +
> +out:
> +       errno = error;
> +       return chip;
> +}
> +
>  static bool isuint(const char *str)
>  {
>         for (; *str && isdigit(*str); str++)
> @@ -166,6 +227,8 @@ struct gpiod_chip *chip_open_lookup(const char *device)
>                 else
>                         chip = gpiod_chip_open(device);
>         }
> +       if (!chip)
> +               chip = chip_open_by_label(device);
>
>         return chip;
>  }
> --
> 2.20.1