Re: [libgpiod v2][PATCH v3 2/5] tools: line name focussed rework

[Kent Gibson <warthog618@xxxxxxxxx>]

On Wed, Nov 09, 2022 at 12:16:22PM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 9, 2022 at 3:00 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> 
> [snip]
> 
> 
> > >
> > > I insist on this one as just a quick glance at the current values of
> > > gpio-line-names DT properties in the kernel show all kinds of funky
> > > names - not only including spaces but also various other characters
> > > like [, ], #, /, - and that not. I think it makes a lot of sense to
> > > delimit them visually with quotes.
> > >
> >
> > Good grief, what a mess.  Would've been nice to have some naming
> > conventions in place, but too late now...
> >
> > Which reminds me - any guarantees the name, particularly the consumer
> > name, is valid UTF-8?
> > If not then need to double check how the Rust bindings deal with that.
> > Throwing a utf8_error would not be appropriate if it isn't guaranteed to
> > be UTF-8.
> >
> 
> There's nothing that technically prohibits anyone from using non-UTF8
> characters and also current libgpiod seems to be handling such chars
> just fine (just tested to make sure).
> 

Exactly - AIUI the names are simply 32 bytes - any 32 bytes.

There is also the possibility of UTF-8 being truncated mid-character due
to the 32 byte limit - so even if you put UTF-8 in, it might come out
invalid when you read it back.

> > > > >Same for the consumer as with your current
> > > > > version, if the consumer string has spaces in it, it will look like
> > > > > this: consumer=foo bar. I think consumer="foo bar" would be easier to
> > > > > parse.
> > > >
> > > > For this very reason, the consumer is explicitly listed last, so the
> > > > consumer name matches everything between the "consumer=" and end of
> > > > line.
> > > >
> > > > Unless consumer names with spaces are very common in the wild then
> > > > quotes only add more clutter.
> > > >
> > >
> > > We can't know, but instead of putting it last, I'd just treat it like
> > > every other flag and instead delimit the name with "".
> >
> > It is also last based on length predicatability - more predicatable
> > first, and the other attributes are more predictable.
> >
> > >
> > > The tool is mostly aimed at humans anyway and if someone's brave
> > > enough to parse the output with a script then a cut-based one-liner is
> > > all they need, no?
> > >
> >
> > Sure, but don't underestimate the ability of users to complain when a
> > tool isn't quite as easy to use as they would like.
> >
> > Hmmm, how about an option to quote all string attributes?
> > Say --quoted?
> > Or if quoted is the default then --unquoted to remove the clutter?
> >
> 
> I don't feel like it's necessary. How would you handle line names with
> spaces? It would add an additional column? That would be actually
> harder to parse than quoted names IMO.
> 

Line names are surrounded by tabs, so the actual issue is line names
containing tabs. Are there any of those?

And this is just targetting humans (well me at least) anyway.

<snip>
> > > > One thing I was considering was reworking the resolver so it would be
> > > > more suitable for general use, and move it to core libgpiod so apps
> > > > could more readily perform line name discovery.
> > > >
> > >
> > > Hmm, I think it's unnecessary clutter in the library. I was thinking
> > > about whether to put the upcoming fdinfo parsing into the library and
> > > figured that it's more of a suitable candidate for a new command-line
> > > tool as the library should focus on the character device exclusively
> > > IMO.
> > >
> >
> > I think you were right the first time.  The fdinfo is another aspect of
> > the cdev GPIO uAPI - it isn't just the ioctls.
> > If the GPIO uAPI were extended to use AIO, would that not go in libgpiod as
> > well?
> >
> > And agreed that fdinfo warrants another tool.
> >
> 
> I was thinking that both gpioinfo could use that (e.g.
> consumer="foobar",consumer_pid=12345) and we could have something
> like:
> 
> # gpioowner foobar
> 12345
> 

Both those make sense to me.

> > It makes sense to me to add higher level abstractions or functionality
> > where that greatly simplifies apps.
> > Speaking from experience, looking up up the line chip/offset based on
> > name is at best tedious and at worst a serious PITA.
> > If it isn't available in libgpiod then apps are more likely to do it
> > out-of-band, such as by using gpioinfo to generate app configuration,
> > or just stick to specifying lines by chip/offset, which is a bit sad.
> >
> > But, as per the unified tool binary, this can always be added later.
> >
> 
> Ok, I'm buying this. Let's think about adding this to the core library.
> 

Oh great - I should've kept my trap shut ;-).

Cheers,
Kent.