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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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

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

[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. :)

Bart