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

[Ben Hutchings <ben.hutchings@xxxxxxxxxxxxx>]

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@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.
[...]
> > +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