Re: [libgpiod][RFC v2] core: implement v2.0 API

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Jun 01, 2021 at 09:57:08PM +0200, Bartosz Golaszewski wrote:
> On Sun, May 30, 2021 at 2:45 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> 
> [peek]
> 
> > > >
> > >
> > > Eek this sounds vague. If that was part of documentation for this
> > > function - as a user I would be confused. Is there any problem with
> > > just implying output for any line for which a default value is set?
> > >
> >
> > What part of "No need for the user to know or care" is too vague?
> > The user does not need to know how libgpiod translates to uAPI - that
> > is your problem not theirs, so don't go there.
> >
> > But for your benefit...
> >
> > There is no global direction setting - the lines are treated
> > independently until the translation to uAPI during the request or
> > reconfigure call, at which point the most common flags setting can be
> > used as the global default.
> >
> 
> I said "vague" for want of a better word but what I mean is this:
> taking the most common setting as the global default does not sound
> intuitive and -  when coding - it would be hard to predict the outcome
> from just looking at the code. I was actually assuming that we'd have
> lines begin with a certain sane, default config (INPUT, active-high,
> no bias, no drive, no edge etc) and every line's config would have to
> be changed explicitly - either via the global mutators or the ones
> that work with a subset of lines (where the subset could actually
> include all the lines - we don't know it before we do the actual
> request because the list of lines to request is passed in a different
> struct).
> 

Ahh, I forgot that you have the req_config and line_config split.
So you have no option but to maintain global flags.  That is used as
the initial value of the flags for a particular line when that one line is
first mutated, either by offset or as part of a subset.
And subsequent changes to the global flags need to be propagated to all
the lines in the config at that time.

Taking the most common flags value as the uAPI default is an optimization
that minimizes the number of lines requiring custom flag attributes in the
uAPI config. It might seem unintuitive to you, but it makes sense to me
that it should be the uAPI default, even if the user wasn't aware that it
should be.
OTOH there are only a few corner cases where it is of real benefit, so
just using the line_config global flags is fine.
FWIW, my Go library doesn't perform that optimization - it uses the
global flags :).  Not sure why I'm still suggesting it here when I
apparently decided it wasn't worth the effort there ¯\_(ツ)_/¯.

> [snip]
> 
> > >
> > >
> > > > I forgot to ask about where gpiod_line_info_get_name() and others that
> > > > return char * fit wrt that pattern.
> > > > So a string isn't a complex object?
> > > > Maybe they should be _peek_ as well?
> > > > Either way, it would be nice for their commentary to describe the lifetime
> > > > of the returned pointer.
> > >
> > > With strings the common sense is to assume that returning char * means
> > > the string is dynamically allocated and must be freed by the caller,
> > > returning const char * means the string is stored in the container. I
> > > can't really recall seeing any other pattern in any sane C library. In
> > > any case - I will add a comment to every function that returns an
> > > object that needs lifetime management from the caller in v3.
> > >
> >
> > Sure, but don't assume common sense - document the behaviour - even if
> > the returned object doesn't require lifetime management by the caller.
> > They might assume they do - and free it for you.
> >
> > And the point was that you still have some gets that return objects while
> > others return a reference - even after renaming some to peek, so the
> > pattern doesn't follow for the whole libgpiod API. Granted that is
> > nitpicking, but I would prefer internal self-consistency over following
> > what other libraries do.
> >
> 
> I don't want to use "peek" for simple types if that's what you're
> hinting at because that would be very confusing to anyone experienced
> with other C linux libraries. I would like to go with "get" for
> functions returning pointers to opaque structs that need management
> and peek for those that return pointers to structs stored in other
> objects. I don't think we need to have additional "copy" functions
> operating on the copied objects.
> 

Fair enough - just another of those minor points that I would do
differently (I'd just stick with get and live with the fact that
returns ownership in some cases and not in others).

> [snip]
> 
> > > >
> > >
> > > Having error-less mutators here would mean something like a hundred
> > > functions just for setting the line config. I think that even with
> > > mutators taking enums we already have enough symbols. Let's keep them.
> > >
> >
> > We don't want separate mutators for each enum value as that would
> > explode the symbol space.  Agreed.
> >
> > To be clear, in my "trade-off" paragraph above I was referring to your
> > existing parameterised error-less mutators, not the parameter-less
> > error-less mutators that I had been assuming.
> > Different paragraph and different things.
> >
> > So, to conclude, I would lean towards keeping your existing mutators that
> > don't return error codes, rather than adding error codes.
> > And only performing the validation when the config is translated to uAPI,
> > not providing some functions to perform interim validation.
> > i.e. wrt error handling I'm fine with the mutators as they are.
> >
> 
> Ok got it.
> 
> I think we're getting close to an agreement. :)
> 

Well that makes one of us ;).

Cheers,
Kent.