| Message ID | 20210728211916.GB14442@cephalopod |
|---|---|
| State | New |
| Headers | show |
| Series | [libgpiod-v2] tools: Restore support for opening chips by label | expand |
On Wed, Jul 28, 2021 at 11:19 PM Ben Hutchings <ben.hutchings@mind.be> 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@mind.be> > --- > 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
On Mon, Sep 20, 2021 at 04:32:22PM +0200, Bartosz Golaszewski wrote: > On Wed, Jul 28, 2021 at 11:19 PM Ben Hutchings <ben.hutchings@mind.be> 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. [...] > > +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. [...] I don't see any benefit in using access() rather than checking for EACCES; that just seems to add a race condition. As for whether the error should be reported, this is consistent with the old behaviour and I did not want to report that "chip label does not exist" in case of missing permission. And again, I would expect the tools to be run as root, so this shouldn't happen in practice. Perhaps a better approach would be to record any access failure and then report it at the end only if the label was not found? Ben. -- Ben Hutchings · Senior Embedded Software Engineer, Essensium-Mind · mind.be
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)); + 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; }
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@mind.be> --- tools/tools-common.c | 63 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)