[libgpiod PATCH] core: Fix line_bulk_foreach_line invalid memory access

[libgpiod PATCH] core: Fix line_bulk_foreach_line invalid memory access

[Joel Stanley]

Running libgpiod applications under valgrind results in the following
warning:

==3006== Invalid read of size 8
==3006==    at 0x10C867: line_request_values (core.c:711)
==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
==3006==    by 0x10AE27: main (gpioset.c:323)
==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
==3006==    by 0x10ADC3: main (gpioset.c:313)

This is because the foreach loop reads the next value before checking
that index is still in bounds.

Add a test to avoid reading past the end of the allocation.

This bug is not present a released version of libgpiod.

Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 lib/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/core.c b/lib/core.c
index 6ef09baec0f5..4463a7014776 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
 #define line_bulk_foreach_line(bulk, line, index)			\
 	for ((index) = 0, (line) = (bulk)->lines[0];			\
 	     (index) < (bulk)->num_lines;				\
-	     (index)++, (line) = (bulk)->lines[(index)])
+	     (index)++, 						\
+	     (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
 
 GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
 {
-- 
2.34.1

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

[Bartosz Golaszewski]

On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@jms.id.au> wrote:
>
> Running libgpiod applications under valgrind results in the following
> warning:
>
> ==3006== Invalid read of size 8
> ==3006==    at 0x10C867: line_request_values (core.c:711)
> ==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
> ==3006==    by 0x10AE27: main (gpioset.c:323)
> ==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
> ==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
> ==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
> ==3006==    by 0x10ADC3: main (gpioset.c:313)
>
> This is because the foreach loop reads the next value before checking
> that index is still in bounds.
>
> Add a test to avoid reading past the end of the allocation.
>
> This bug is not present a released version of libgpiod.
>
> Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  lib/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/core.c b/lib/core.c
> index 6ef09baec0f5..4463a7014776 100644
> --- a/lib/core.c
> +++ b/lib/core.c
> @@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
>  #define line_bulk_foreach_line(bulk, line, index)                      \
>         for ((index) = 0, (line) = (bulk)->lines[0];                    \
>              (index) < (bulk)->num_lines;                               \
> -            (index)++, (line) = (bulk)->lines[(index)])
> +            (index)++,                                                 \
> +            (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
>
>  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
>  {
> --
> 2.34.1
>

I'll skip this because this entire struct is going away in v2 and the
bug is not present in v1.6.x.

Bart

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

[Bartosz Golaszewski]

On Fri, Feb 18, 2022 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@jms.id.au> wrote:
> >
> > Running libgpiod applications under valgrind results in the following
> > warning:
> >
> > ==3006== Invalid read of size 8
> > ==3006==    at 0x10C867: line_request_values (core.c:711)
> > ==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
> > ==3006==    by 0x10AE27: main (gpioset.c:323)
> > ==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
> > ==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> > ==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
> > ==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
> > ==3006==    by 0x10ADC3: main (gpioset.c:313)
> >
> > This is because the foreach loop reads the next value before checking
> > that index is still in bounds.
> >
> > Add a test to avoid reading past the end of the allocation.
> >
> > This bug is not present a released version of libgpiod.
> >
> > Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> >  lib/core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/core.c b/lib/core.c
> > index 6ef09baec0f5..4463a7014776 100644
> > --- a/lib/core.c
> > +++ b/lib/core.c
> > @@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
> >  #define line_bulk_foreach_line(bulk, line, index)                      \
> >         for ((index) = 0, (line) = (bulk)->lines[0];                    \
> >              (index) < (bulk)->num_lines;                               \
> > -            (index)++, (line) = (bulk)->lines[(index)])
> > +            (index)++,                                                 \
> > +            (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
> >
> >  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
> >  {
> > --
> > 2.34.1
> >
>
> I'll skip this because this entire struct is going away in v2 and the
> bug is not present in v1.6.x.
>
> Bart

Ugh actually all three patches fix issues in the master branch that
have never been nor will be released.

I'm not sure if I made myself clear on that - the changes in the
master branch are going away and the de facto new API is in
next/libgpiod-2.0. I already pushed the other two so I'll leave them
there but please take a look at the next branch so that you know how
the upcoming API will work. That's also applicable to the patches
adding the by-name option to the tools - I think it would be better to
base them on that branch right away.

Bart

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

[Joel Stanley]

On Fri, 18 Feb 2022 at 18:42, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Feb 18, 2022 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@jms.id.au> wrote:
> > >
> > > Running libgpiod applications under valgrind results in the following
> > > warning:
> > >
> > > ==3006== Invalid read of size 8
> > > ==3006==    at 0x10C867: line_request_values (core.c:711)
> > > ==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
> > > ==3006==    by 0x10AE27: main (gpioset.c:323)
> > > ==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
> > > ==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > ==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
> > > ==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
> > > ==3006==    by 0x10ADC3: main (gpioset.c:313)
> > >
> > > This is because the foreach loop reads the next value before checking
> > > that index is still in bounds.
> > >
> > > Add a test to avoid reading past the end of the allocation.
> > >
> > > This bug is not present a released version of libgpiod.
> > >
> > > Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
> > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > ---
> > >  lib/core.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/core.c b/lib/core.c
> > > index 6ef09baec0f5..4463a7014776 100644
> > > --- a/lib/core.c
> > > +++ b/lib/core.c
> > > @@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
> > >  #define line_bulk_foreach_line(bulk, line, index)                      \
> > >         for ((index) = 0, (line) = (bulk)->lines[0];                    \
> > >              (index) < (bulk)->num_lines;                               \
> > > -            (index)++, (line) = (bulk)->lines[(index)])
> > > +            (index)++,                                                 \
> > > +            (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
> > >
> > >  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
> > >  {
> > > --
> > > 2.34.1
> > >
> >
> > I'll skip this because this entire struct is going away in v2 and the
> > bug is not present in v1.6.x.
> >
> > Bart
>
> Ugh actually all three patches fix issues in the master branch that
> have never been nor will be released.
>
> I'm not sure if I made myself clear on that - the changes in the
> master branch are going away and the de facto new API is in
> next/libgpiod-2.0. I already pushed the other two so I'll leave them
> there but please take a look at the next branch so that you know how
> the upcoming API will work. That's also applicable to the patches
> adding the by-name option to the tools - I think it would be better to
> base them on that branch right away.

That's a bit frustrating.

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?

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

[Bartosz Golaszewski]

On Thu, Feb 24, 2022 at 2:15 AM Joel Stanley <joel@jms.id.au> wrote:
>
> On Fri, 18 Feb 2022 at 18:42, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > On Fri, Feb 18, 2022 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@jms.id.au> wrote:
> > > >
> > > > Running libgpiod applications under valgrind results in the following
> > > > warning:
> > > >
> > > > ==3006== Invalid read of size 8
> > > > ==3006==    at 0x10C867: line_request_values (core.c:711)
> > > > ==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
> > > > ==3006==    by 0x10AE27: main (gpioset.c:323)
> > > > ==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
> > > > ==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > > ==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
> > > > ==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
> > > > ==3006==    by 0x10ADC3: main (gpioset.c:313)
> > > >
> > > > This is because the foreach loop reads the next value before checking
> > > > that index is still in bounds.
> > > >
> > > > Add a test to avoid reading past the end of the allocation.
> > > >
> > > > This bug is not present a released version of libgpiod.
> > > >
> > > > Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
> > > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > > ---
> > > >  lib/core.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/lib/core.c b/lib/core.c
> > > > index 6ef09baec0f5..4463a7014776 100644
> > > > --- a/lib/core.c
> > > > +++ b/lib/core.c
> > > > @@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
> > > >  #define line_bulk_foreach_line(bulk, line, index)                      \
> > > >         for ((index) = 0, (line) = (bulk)->lines[0];                    \
> > > >              (index) < (bulk)->num_lines;                               \
> > > > -            (index)++, (line) = (bulk)->lines[(index)])
> > > > +            (index)++,                                                 \
> > > > +            (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
> > > >
> > > >  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
> > > >  {
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > I'll skip this because this entire struct is going away in v2 and the
> > > bug is not present in v1.6.x.
> > >
> > > Bart
> >
> > Ugh actually all three patches fix issues in the master branch that
> > have never been nor will be released.
> >
> > I'm not sure if I made myself clear on that - the changes in the
> > master branch are going away and the de facto new API is in
> > next/libgpiod-2.0. I already pushed the other two so I'll leave them
> > there but please take a look at the next branch so that you know how
> > the upcoming API will work. That's also applicable to the patches
> > adding the by-name option to the tools - I think it would be better to
> > base them on that branch right away.
>
> 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?

Thank you for your understanding,
Bart

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

[Kent Gibson]

On Thu, Feb 24, 2022 at 03:50:18PM +0100, Bartosz Golaszewski wrote:
> On Thu, Feb 24, 2022 at 2:15 AM Joel Stanley <joel@jms.id.au> wrote:
> >
> > On Fri, 18 Feb 2022 at 18:42, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > On Fri, Feb 18, 2022 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > On Wed, Feb 2, 2022 at 12:42 PM Joel Stanley <joel@jms.id.au> wrote:
> > > > >
> > > > > Running libgpiod applications under valgrind results in the following
> > > > > warning:
> > > > >
> > > > > ==3006== Invalid read of size 8
> > > > > ==3006==    at 0x10C867: line_request_values (core.c:711)
> > > > > ==3006==    by 0x10CDA6: gpiod_line_request_bulk (core.c:849)
> > > > > ==3006==    by 0x10AE27: main (gpioset.c:323)
> > > > > ==3006==  Address 0x4a4d370 is 0 bytes after a block of size 16 alloc'd
> > > > > ==3006==    at 0x483F790: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > > > ==3006==    by 0x10B884: gpiod_line_bulk_new (core.c:109)
> > > > > ==3006==    by 0x10DBB0: gpiod_chip_get_lines (helpers.c:24)
> > > > > ==3006==    by 0x10ADC3: main (gpioset.c:313)
> > > > >
> > > > > This is because the foreach loop reads the next value before checking
> > > > > that index is still in bounds.
> > > > >
> > > > > Add a test to avoid reading past the end of the allocation.
> > > > >
> > > > > This bug is not present a released version of libgpiod.
> > > > >
> > > > > Fixes: 2b02d7ae1aa6 ("treewide: rework struct gpiod_line_bulk")
> > > > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > > > ---
> > > > >  lib/core.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/lib/core.c b/lib/core.c
> > > > > index 6ef09baec0f5..4463a7014776 100644
> > > > > --- a/lib/core.c
> > > > > +++ b/lib/core.c
> > > > > @@ -178,7 +178,8 @@ GPIOD_API void gpiod_line_bulk_foreach_line(struct gpiod_line_bulk *bulk,
> > > > >  #define line_bulk_foreach_line(bulk, line, index)                      \
> > > > >         for ((index) = 0, (line) = (bulk)->lines[0];                    \
> > > > >              (index) < (bulk)->num_lines;                               \
> > > > > -            (index)++, (line) = (bulk)->lines[(index)])
> > > > > +            (index)++,                                                 \
> > > > > +            (line) = (index) < (bulk)->num_lines ? (bulk)->lines[(index)] : NULL)
> > > > >
> > > > >  GPIOD_API bool gpiod_is_gpiochip_device(const char *path)
> > > > >  {
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > > > I'll skip this because this entire struct is going away in v2 and the
> > > > bug is not present in v1.6.x.
> > > >
> > > > Bart
> > >
> > > Ugh actually all three patches fix issues in the master branch that
> > > have never been nor will be released.
> > >
> > > I'm not sure if I made myself clear on that - the changes in the
> > > master branch are going away and the de facto new API is in
> > > next/libgpiod-2.0. I already pushed the other two so I'll leave them
> > > there but please take a look at the next branch so that you know how
> > > the upcoming API will work. That's also applicable to the patches
> > > adding the by-name option to the tools - I think it would be better to
> > > base them on that branch right away.
> >
> > 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.

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?

The sets should also accept a set of true/false variants, such as the
on/off, active/inactive in the examples above.
The gets should return active/inactive to make it clear they refer to
logical values, not physical values.

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

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.

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.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> 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.

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

[Kent Gibson]

On Fri, Feb 25, 2022 at 10:55:25PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> 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.
> 

Oh, I know - I've made that same point myself.
But it cuts both ways - if you don't fix things now you never will.
Make current users angry, or make future users confused and angry.
Your call.

Of course my prefered option is an interface that will satisfy both
current and future users, but I seriously doubt we can get there if we
maintain backward compatibility for the sake of current users.

> > 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.
> 

I'm thinking that most users would prefer to work with names.
And that preference will only grow stronger over time.
I'm starting to lean that way myself.

So you are happy with positional chip by default, but optional when
--by-name is set?  That is going to make command line parsing even more
fun, but ok.

So your acceptible forms would be (using get for brevity):
    gpioget gpiochip0 16 17 23
    gpioget --by-name GPIO16 GPIO17 GPIO23
    gpioget --by-name -c gpiochip0 GPIO16 GPIO17 GPIO23
 
?

> > 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.
> 

All kinds?  I suggested two variants.  That is all, not the whole thesaurus.
I explicitly avoid 0/1 and low/high, cos line levels.
Could be convinces to accept 0/1 for backward compatibility.

Users can then use a nomenclature that best suits their application.
That makes their scripts more readable.  And much more meaningful than
0/1. I mean 0/1 is literally the least amount of information you could
possibly provide.

While the active-low option exists, setting line=1 will always be
confusing.  Dropping the active-low option for set would mean it
operates with physical levels, while the other commands use logical,
so don't really want to go there either....

(I have also wondered if having the active-low option in the uAPI
is actually beneficial in general, but that bird has flown.)

> > The gets should return active/inactive to make it clear they refer to
> > logical values, not physical values.
> >
> 
> What's wrong with 1/0?
> 

As stated, that can easily be misinterpreted as meaning physical line levels.
And the logical and physical levels are not the same if active-low,
and the get command gives no indication if the active-low is set or not.

> > 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?
> 

One advantage of a single tool is that when you add a new one, say
watch, it just gets added to the tool and pops up in the tool help.
So it aids discoverabilty.

And adding commands doesn't automatically require an extra binary to be
built and installed - just upgrading the one.

The busy-box command-link approach is an option, with the link name
determining the subcommand. Or it can be called directly with subcommands.
e.g. gpiodctl set gpiochip0 16=0

Wrt backward compatibility, perhaps add an option to behave as per v1,
so current users can just add that until they migrate to the v2 command
line?
When called directly it could use the v2 command line, but when called
via a link, it could enable the v1 option and support the v1 command line.
Or is that just confusing too?

> 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.
> 

Who needs the README - the tool help should be sufficient.
If not then fix the help.

They might be tiny, but collectively they can't be smaller than a unified
executable.
Are there cases where only a subset of commands is deployed?
Though even that could be addressed with build options to drop subcommands
from the tool.

> > 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.
> 

Yeah, it also solves most of my discomfort with the named option - it
only does the find once and caches the offset for subsequent sets.
The best of both worlds.

Would adding a find ioctl to the uAPI be worthwhile?

> > 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?
> 

Other than the named mode...

The set is the one that bothers me the most.
The --mode is a bit confusing. Most of them map to the same thing,
and I can get the same effect with --interactive and/or a hold period.
(the hold period being a forced sleep after a set.  That comes in handy
in the interactive mode where you can just hammer out sets and the hold
period defines the rate they will be applied, so blinking or even
bit-bashing becomes super easy)

Time periods should be a single option that includes the time units, 
e.g. -p 10ms is more readable than -u 10000000 (how many zeros is that??)

And I'm pondering a toggle/blink mode, which would toggle the requested
lines at the rate defined by the hold period.  So a simple blinker would
be:
    gpioset --by-name --blink -p 1s LED=on

And a corresponding toggle command for the interactive mode.
(which could specify a subset of lines to toggle, or toggle the whole
set)

If you want to retain backward compatibility then adding these
features, without removing the old, will produce a cluttered, messy
and really confusing gpioset interface.  And that is the tool most
likely to see use :(.

Wrt gpioinfo, I'm not sure the quotes on the name and consumer in the
output helps.  And I'd restrict the chip option to a single chip, else
all chips.  No subset option, to be consistent with other tools
and to allow the option of selecting the line or a subset of lines,
either by offset or name.
e.g. who is consuming the LED line?
    gpioinfo --by-name LED

gpiofind should have a chip option to limit searching (names are only
unique within chips, right, or is even that still not guaranteed?),
and an option to provide the info for found lines - since we have it in
hand anyway.

gpiodetect could get a chip option, so only get the info for the chip
of interest (to ignore those orphaned gpio-sims ;-).

gpiomon should provide sequence numbers.  Maybe an option to return the
whole event in JSON?

Anyway, in short, if backward compatibility is a requirement then I'm
really not sure how best to proceed - except now I need another coffee.

Cheers,
Kent.

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

[Bartosz Golaszewski]

On Sat, Feb 26, 2022 at 5:01 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Feb 25, 2022 at 10:55:25PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> 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.
> >
>
> Oh, I know - I've made that same point myself.
> But it cuts both ways - if you don't fix things now you never will.
> Make current users angry, or make future users confused and angry.
> Your call.
>
> Of course my prefered option is an interface that will satisfy both
> current and future users, but I seriously doubt we can get there if we
> maintain backward compatibility for the sake of current users.
>

You opened a can of worms below. :)

> > > 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.
> >
>
> I'm thinking that most users would prefer to work with names.
> And that preference will only grow stronger over time.
> I'm starting to lean that way myself.
>

Ok, I may get convinced but we'd need something that behaves
consistently and predictably. A GPIO line name can still parse as an
unsigned integer so we cannot make assumptions here. Maybe --by-name
or --by-offset should always be required?

> So you are happy with positional chip by default, but optional when
> --by-name is set?  That is going to make command line parsing even more
> fun, but ok.
>
> So your acceptible forms would be (using get for brevity):
>     gpioget gpiochip0 16 17 23
>     gpioget --by-name GPIO16 GPIO17 GPIO23
>     gpioget --by-name -c gpiochip0 GPIO16 GPIO17 GPIO23
>
> ?
>
> > > 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.
> >
>
> All kinds?  I suggested two variants.  That is all, not the whole thesaurus.
> I explicitly avoid 0/1 and low/high, cos line levels.
> Could be convinces to accept 0/1 for backward compatibility.
>
> Users can then use a nomenclature that best suits their application.
> That makes their scripts more readable.  And much more meaningful than
> 0/1. I mean 0/1 is literally the least amount of information you could
> possibly provide.
>

Do you think we should also use this naming in the library? As in:

enum {
    GPIOD_LINE_STATE_ACTIVE = 1,
    GPIOD_LINE_STATE_INACTIVE = 0
};

And corresponding scoped enums in C++ and Python?

> While the active-low option exists, setting line=1 will always be
> confusing.  Dropping the active-low option for set would mean it
> operates with physical levels, while the other commands use logical,
> so don't really want to go there either....
>
> (I have also wondered if having the active-low option in the uAPI
> is actually beneficial in general, but that bird has flown.)
>
> > > The gets should return active/inactive to make it clear they refer to
> > > logical values, not physical values.
> > >
> >
> > What's wrong with 1/0?
> >
>
> As stated, that can easily be misinterpreted as meaning physical line levels.
> And the logical and physical levels are not the same if active-low,
> and the get command gives no indication if the active-low is set or not.
>

Right.

> > > 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?
> >
>
> One advantage of a single tool is that when you add a new one, say
> watch, it just gets added to the tool and pops up in the tool help.
> So it aids discoverabilty.
>
> And adding commands doesn't automatically require an extra binary to be
> built and installed - just upgrading the one.
>
> The busy-box command-link approach is an option, with the link name
> determining the subcommand. Or it can be called directly with subcommands.
> e.g. gpiodctl set gpiochip0 16=0
>

I can work with that.

> Wrt backward compatibility, perhaps add an option to behave as per v1,
> so current users can just add that until they migrate to the v2 command
> line?
> When called directly it could use the v2 command line, but when called
> via a link, it could enable the v1 option and support the v1 command line.
> Or is that just confusing too?
>
> > 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.
> >
>
> Who needs the README - the tool help should be sufficient.
> If not then fix the help.
>
> They might be tiny, but collectively they can't be smaller than a unified
> executable.
> Are there cases where only a subset of commands is deployed?
> Though even that could be addressed with build options to drop subcommands
> from the tool.
>

Indeed.

> > > 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.
> >
>
> Yeah, it also solves most of my discomfort with the named option - it
> only does the find once and caches the offset for subsequent sets.
> The best of both worlds.
>
> Would adding a find ioctl to the uAPI be worthwhile?
>

I don't think so and the reason for that is this: if you want maximum
performance, you should be ready to spend time and use the C API. The
command line tools aren't meant to be fast. I suppose you spend more
time forking and execing than looking up names frankly. The DBus API
and gpioset --interactive will most likely solve this issue anyway.

> > > 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.
> > >

Ok, let's think about what we must do before v2 and what can be
implemented later without breaking compatibility again.

> >
> > 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?
> >
>
> Other than the named mode...
>
> The set is the one that bothers me the most.
> The --mode is a bit confusing. Most of them map to the same thing,
> and I can get the same effect with --interactive and/or a hold period.
> (the hold period being a forced sleep after a set.  That comes in handy
> in the interactive mode where you can just hammer out sets and the hold
> period defines the rate they will be applied, so blinking or even
> bit-bashing becomes super easy)

Given that - as someone pointed out recently - some users find it
difficult to "gpioget `gpiofind GPIO0`" (hence the whole --by-name
idea) I expect they would find it even harder to fiddle with pipes and
other shellnanigans.

>
> Time periods should be a single option that includes the time units,
> e.g. -p 10ms is more readable than -u 10000000 (how many zeros is that??)
>
> And I'm pondering a toggle/blink mode, which would toggle the requested
> lines at the rate defined by the hold period.  So a simple blinker would
> be:
>     gpioset --by-name --blink -p 1s LED=on
>
> And a corresponding toggle command for the interactive mode.
> (which could specify a subset of lines to toggle, or toggle the whole
> set)
>
> If you want to retain backward compatibility then adding these
> features, without removing the old, will produce a cluttered, messy
> and really confusing gpioset interface.  And that is the tool most
> likely to see use :(.
>

Doesn't really look like it to me for the three examples above, but
I'm not entirely against non-compatible changes. I would just prefer
to keep a similar look and feel.

> Wrt gpioinfo, I'm not sure the quotes on the name and consumer in the
> output helps.

You mean for programmatic parsing? For a human reader this doesn't
really matter, does it?

> And I'd restrict the chip option to a single chip, else
> all chips.  No subset option, to be consistent with other tools
> and to allow the option of selecting the line or a subset of lines,
> either by offset or name.
> e.g. who is consuming the LED line?
>     gpioinfo --by-name LED
>
> gpiofind should have a chip option to limit searching (names are only
> unique within chips, right, or is even that still not guaranteed?),
> and an option to provide the info for found lines - since we have it in
> hand anyway.
>
> gpiodetect could get a chip option, so only get the info for the chip
> of interest (to ignore those orphaned gpio-sims ;-).
>

Touché.

> gpiomon should provide sequence numbers.  Maybe an option to return the
> whole event in JSON?
>

Sure, sequence numbers are already on my TODO list just like any other
new information that's passed over v2 ABI.

> Anyway, in short, if backward compatibility is a requirement then I'm
> really not sure how best to proceed - except now I need another coffee.
>

Let's say that I'm open to breaking changes but I would like to limit
them wherever we can unlike the library.

I won't get to it though before we merge v2 API into master, so going
back to writing C++ tests. :)

Bart

> Cheers,
> Kent.

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

[Kent Gibson]

On Sat, Feb 26, 2022 at 03:34:19PM +0100, Bartosz Golaszewski wrote:
> On Sat, Feb 26, 2022 at 5:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Feb 25, 2022 at 10:55:25PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Feb 25, 2022 at 4:08 PM Kent Gibson <warthog618@gmail.com> 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.
> > >
> >
> > Oh, I know - I've made that same point myself.
> > But it cuts both ways - if you don't fix things now you never will.
> > Make current users angry, or make future users confused and angry.
> > Your call.
> >
> > Of course my prefered option is an interface that will satisfy both
> > current and future users, but I seriously doubt we can get there if we
> > maintain backward compatibility for the sake of current users.
> >
> 
> You opened a can of worms below. :)
> 

Well everyone should be good at something.

> > > > 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.
> > >
> >
> > I'm thinking that most users would prefer to work with names.
> > And that preference will only grow stronger over time.
> > I'm starting to lean that way myself.
> >
> 
> Ok, I may get convinced but we'd need something that behaves
> consistently and predictably. A GPIO line name can still parse as an
> unsigned integer so we cannot make assumptions here. Maybe --by-name
> or --by-offset should always be required?
> 

Agreed, that is why I've been kicking around alternatives.

There really are cases of lines with integer names?
That don't match the offset?
Then we would need an option to force --by-name, even if the name parses
as an integer.

Don't like the mandatory option idea - the most common use case
shouldn't require options.

If you are going to have mandatory options then you might as well have
two separate subcommands, such as set_by_name and set_by_offset.
Not that I am suggesting that - just saying.
 
> > So you are happy with positional chip by default, but optional when
> > --by-name is set?  That is going to make command line parsing even more
> > fun, but ok.
> >
> > So your acceptible forms would be (using get for brevity):
> >     gpioget gpiochip0 16 17 23
> >     gpioget --by-name GPIO16 GPIO17 GPIO23
> >     gpioget --by-name -c gpiochip0 GPIO16 GPIO17 GPIO23
> >
> > ?
> >
> > > > 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.
> > >
> >
> > All kinds?  I suggested two variants.  That is all, not the whole thesaurus.
> > I explicitly avoid 0/1 and low/high, cos line levels.
> > Could be convinces to accept 0/1 for backward compatibility.
> >
> > Users can then use a nomenclature that best suits their application.
> > That makes their scripts more readable.  And much more meaningful than
> > 0/1. I mean 0/1 is literally the least amount of information you could
> > possibly provide.
> >
> 
> Do you think we should also use this naming in the library? As in:
> 
> enum {
>     GPIOD_LINE_STATE_ACTIVE = 1,
>     GPIOD_LINE_STATE_INACTIVE = 0
> };
> 

Yup, though all the functions return ints, so the user is free to
overlook that enum.

OTOH, my Rust library returns one of these from a get_value():

pub enum Value {
    /// The line is inactive.
    Inactive,
    /// The line is active.
    Active,
}

So no 0 or 1 anywhere, and no possible confusion.
Well, hopefully.

> And corresponding scoped enums in C++ and Python?
> 
> > While the active-low option exists, setting line=1 will always be
> > confusing.  Dropping the active-low option for set would mean it
> > operates with physical levels, while the other commands use logical,
> > so don't really want to go there either....
> >
> > (I have also wondered if having the active-low option in the uAPI
> > is actually beneficial in general, but that bird has flown.)
> >
> > > > The gets should return active/inactive to make it clear they refer to
> > > > logical values, not physical values.
> > > >
> > >
> > > What's wrong with 1/0?
> > >
> >
> > As stated, that can easily be misinterpreted as meaning physical line levels.
> > And the logical and physical levels are not the same if active-low,
> > and the get command gives no indication if the active-low is set or not.
> >
> 
> Right.
> 
> > > > 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?
> > >
> >
> > One advantage of a single tool is that when you add a new one, say
> > watch, it just gets added to the tool and pops up in the tool help.
> > So it aids discoverabilty.
> >
> > And adding commands doesn't automatically require an extra binary to be
> > built and installed - just upgrading the one.
> >
> > The busy-box command-link approach is an option, with the link name
> > determining the subcommand. Or it can be called directly with subcommands.
> > e.g. gpiodctl set gpiochip0 16=0
> >
> 
> I can work with that.
> 
> > Wrt backward compatibility, perhaps add an option to behave as per v1,
> > so current users can just add that until they migrate to the v2 command
> > line?
> > When called directly it could use the v2 command line, but when called
> > via a link, it could enable the v1 option and support the v1 command line.
> > Or is that just confusing too?
> >
> > > 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.
> > >
> >
> > Who needs the README - the tool help should be sufficient.
> > If not then fix the help.
> >
> > They might be tiny, but collectively they can't be smaller than a unified
> > executable.
> > Are there cases where only a subset of commands is deployed?
> > Though even that could be addressed with build options to drop subcommands
> > from the tool.
> >
> 
> Indeed.
> 
> > > > 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.
> > >
> >
> > Yeah, it also solves most of my discomfort with the named option - it
> > only does the find once and caches the offset for subsequent sets.
> > The best of both worlds.
> >
> > Would adding a find ioctl to the uAPI be worthwhile?
> >
> 
> I don't think so and the reason for that is this: if you want maximum
> performance, you should be ready to spend time and use the C API. The
> command line tools aren't meant to be fast. I suppose you spend more
> time forking and execing than looking up names frankly. The DBus API
> and gpioset --interactive will most likely solve this issue anyway.
>

Yeah, or they can switch to --by-offset.  Good call.

> > > > 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.
> > > >
> 
> Ok, let's think about what we must do before v2 and what can be
> implemented later without breaking compatibility again.
> 

I think this is doable in the timeframe you have in mind, so I would be
hoping to have it completed for v2.  The big sticking point is agreeing on
the command line options and behaviours.  The implementation should be
pretty straight forward given that.

> > >
> > > 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?
> > >
> >
> > Other than the named mode...
> >
> > The set is the one that bothers me the most.
> > The --mode is a bit confusing. Most of them map to the same thing,
> > and I can get the same effect with --interactive and/or a hold period.
> > (the hold period being a forced sleep after a set.  That comes in handy
> > in the interactive mode where you can just hammer out sets and the hold
> > period defines the rate they will be applied, so blinking or even
> > bit-bashing becomes super easy)
> 
> Given that - as someone pointed out recently - some users find it
> difficult to "gpioget `gpiofind GPIO0`" (hence the whole --by-name
> idea) I expect they would find it even harder to fiddle with pipes and
> other shellnanigans.
> 

Different use cases.  They just wanted to get a line by name.

Whereas I'm talking about someone wanting to quickly prototype a driver
for an LED string or stepper motor or something.
So a more complex use case and so more likely to put in the extra effort.
And helping to cover the ground where pulling out a full on programming
language API feels like overkill.

> >
> > Time periods should be a single option that includes the time units,
> > e.g. -p 10ms is more readable than -u 10000000 (how many zeros is that??)
> >
> > And I'm pondering a toggle/blink mode, which would toggle the requested
> > lines at the rate defined by the hold period.  So a simple blinker would
> > be:
> >     gpioset --by-name --blink -p 1s LED=on
> >
> > And a corresponding toggle command for the interactive mode.
> > (which could specify a subset of lines to toggle, or toggle the whole
> > set)
> >
> > If you want to retain backward compatibility then adding these
> > features, without removing the old, will produce a cluttered, messy
> > and really confusing gpioset interface.  And that is the tool most
> > likely to see use :(.
> >
> 
> Doesn't really look like it to me for the three examples above, but
> I'm not entirely against non-compatible changes. I would just prefer
> to keep a similar look and feel.
> 
> > Wrt gpioinfo, I'm not sure the quotes on the name and consumer in the
> > output helps.
> 
> You mean for programmatic parsing? For a human reader this doesn't
> really matter, does it?
> 

I am actually concerned about the human reader.
Is this easy to read:

	line  41:  "PWM1_MOSI"       unused   input  active-high
	line  42: "STATUS_LED_G_CLK" "led0" output active-high [used]
	line  43: "SPIFLASH_CE_N" unused input active-high
	line  44:       "SDA0"       unused   input  active-high
	line  45:       "SCL0"       unused   input  active-high
	line  46: "RGMII_RXCLK" unused input active-high

As unquoted unnamed and unused are possible, parsing the current
line is non-trivial.
Is there actually a use case for parsing the name and consumer?

I replaced the quotes with tabs in my gpioinfo.  It is easier for a
human to parse, and programmatically as well:

	line  41:	PWM1_MOSI       	unused      [input]
	line  42:	STATUS_LED_G_CLK	led0        [output, used]
	line  43:	SPIFLASH_CE_N   	unused      [input]
	line  44:	SDA0            	unused      [input]
	line  45:	SCL0            	unused      [input]
	line  46:	RGMII_RXCLK     	unused      [input]

Or are there names containing tabs?
Any lines named "unnamed" and consumers called "unused" could be
problematic, but anyone doing that deserves what they get.

Oh, and I find the left alignment is more readable for lines with the
same prefix, e.g. SPI_xxx

> > And I'd restrict the chip option to a single chip, else
> > all chips.  No subset option, to be consistent with other tools
> > and to allow the option of selecting the line or a subset of lines,
> > either by offset or name.
> > e.g. who is consuming the LED line?
> >     gpioinfo --by-name LED
> >
> > gpiofind should have a chip option to limit searching (names are only
> > unique within chips, right, or is even that still not guaranteed?),
> > and an option to provide the info for found lines - since we have it in
> > hand anyway.
> >
> > gpiodetect could get a chip option, so only get the info for the chip
> > of interest (to ignore those orphaned gpio-sims ;-).
> >
> 
> Touché.
> 
> > gpiomon should provide sequence numbers.  Maybe an option to return the
> > whole event in JSON?
> >
> 
> Sure, sequence numbers are already on my TODO list just like any other
> new information that's passed over v2 ABI.
> 
> > Anyway, in short, if backward compatibility is a requirement then I'm
> > really not sure how best to proceed - except now I need another coffee.
> >
> 
> Let's say that I'm open to breaking changes but I would like to limit
> them wherever we can unlike the library.
> 
> I won't get to it though before we merge v2 API into master, so going
> back to writing C++ tests. :)
> 

Fair enough.  I wasn't expecting you to work on it - I was thinking it
was something that I would work on in parallel.  Or that maybe Joel
would be interested in chipping in on.  Even getting feedback on the
command line options from Joel or other gpiod users would be helpful.
I suppose we should hash out a proposal that we are ok with and then
post an RFC for general comment?

Cheers,
Kent.