Re: [libgpiod PATCH] core: Fix line_bulk_foreach_line invalid memory access

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>

[snip]

> > >
> > > That's a bit frustrating.
> > >
> >
> > I know and I'm sorry. I admit that this is not the best time to try to
> > get new features in.
> >
> > > Perhaps you could make the master branch contain the code you're
> > > working on (instead of next), if you plan on abandoning the current
> > > code base?
> >
> > I can't just yet. I want to keep the codebase bisectable and only
> > merge the new API into master once it's complete for the C, C++ and
> > Python parts. The branch called next/libgpiod-2.0 contains the WIP
> > changes but they are not complete yet. I just posted the test suite
> > for C and plan on posting the complete C++ bindings soon.
> >
> > In fact - we discussed it with Kent and Linus and I expect to be able
> > to release the v2 in around two months and merge the new API into
> > master in a month.
> >
> > You can base your work on next/libgpiod-2.0 but could you just hold
> > off the new features until after the new API is in master?
> >
>
> I'm thinking that we should be re-visting the tools as part of the
> switch to libgpiod v2, as a major version bump is our only opportunity
> to make sweeping changes.
>

Yes and no. I'm not very happy about making the very command-line
users that we managed to convince to switch away from sysfs to using
gpio-tools angry again with totally incompatible changes in v2.

> I have to admit I was not initially in favour of the by-name option, as
> it is hideously inefficient compared to the offset version.  What was
> one or two ioctl calls could now be dozens, if not more.
> And the thought of that happening everytime a user wants to toggle a
> single line makes my skin crawl.
>
> However, in light of our recent discussions, I think we need it as an
> option.  But I would prefer to revise the tool command lines so lines
> can be identified by name or offset.  The named option should be the
> simplest, and so not require a --by-name flag.
> My current thinking is that the chip should become an optional arg,
> rather than a positional arg.
> So [-c <chip>] <line>...
> e.g.
>     gpioset GPIO17=active GPIO22=1
> or
>     gpioset -c gpiochip0 17=1 LED=off
>
> similarly get:
>
>     gpioget GPIO17 GPIO22
> or
>     gpioget -c gpiochip0 17 LED
>
> If the chip is not specified then the line identifier must be a name.
> If the chip is specified then the line identifier is interpreted as an
> offset if it parses as an int, else a name.
> Either way, if multiple lines are provided then they must be on the one
> chip.
> That all hinges on the assumption that line names are never simply
> stringified integers, or at least if they are then it matches the
> offset.  Is that viable?
>

We cannot make that assumptions and I would prefer to stay both
compatible AND explicit here. As in: work with offsets by default and
with names as an option. On the other hand - if we specify --by-name,
I'm fine with skipping the chip parameter and let the program look up
the line among all chips.

> The sets should also accept a set of true/false variants, such as the
> on/off, active/inactive in the examples above.

Why though? What do we gain from accepting all kinds of different
strings? IMO it just makes the interface less clear.

> The gets should return active/inactive to make it clear they refer to
> logical values, not physical values.
>

What's wrong with 1/0?

> I am also wondering why the tools are separate, instead of being merged
> into a single tool.  Is there a reason for that?
>

You mean like busybox' single executable with multiple links under
different names internally redirecting to different main() functions
or really a single tool with multiple commands?

The reason for having separate tools is that they really are tiny, the
little code they share and statically link to is negligible and it's
simply clearer as to which tool does what. I didn't want the tools to
be this swiss-knife do-it-all program that requires studying the
README for a long time.

> I've got a bunch of other minor changes that I've been trialing in my
> Rust library.  So I have a working prototype of the set --interactive
> that I had mentioned.  I scrapped the batch option - it doesn't
> add much that the interactive mode and a named pipe doesn't already give
> you.
>

gpioset --interactive is definitely something I'm interested in.

> But I digress.  The main thing I want to achieve here is to determine
> where you want to go with the tools for v2, and what any contraints
> might be.  Then we can take it from there.
>

While the total overhaul of the library is understandable, I would
prefer to keep the tools mostly backward compatible. I plan on adding
gpiowatch for watching info events but that's it. Do you see any
things that are obviously wrong in how the tools work that would
justify the sweeping changing you mention?

Bart

> Cheers,
> Kent.