Re: [libgpiod][PATCH] treewide: rework struct gpiod_line_bulk

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Oct 24, 2020 at 10:01 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Oct 23, 2020 at 11:28:31AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> >
>
> [snip]
>
> > @@ -74,6 +74,106 @@ struct gpiod_chip {
> >       char label[32];
> >  };
> >
> > +/*
> > + * The structure is defined in a way that allows internal users to allocate
> > + * bulk objects that hold a single line on the stack - that way we can reuse
> > + * a lot of code between functions that take single lines and those that take
> > + * bulks as arguments while not unnecessarily allocating memory dynamically.
> > + */
> > +struct gpiod_line_bulk {
> > +     struct gpiod_chip *owner;
> > +     unsigned int num_lines;
> > +     unsigned int max_lines;
> > +     struct gpiod_line *lines[1];
> > +};
> > +
>
> owner is only used to check that added lines are on same chip.
> Just compare with lines[0]->chip in gpiod_line_bulk_add_line()
> instead?
>

Yes, of course.

> Also, line_bulk_same_chip() is now redundant as lines can only be added
> to the bulk via gpiod_line_bulk_add_line() which performs the check,
> so remove it and all uses of it throughout.
>

Sure.

[snip]

>
> I question the value of the struct gpiod_line_bulk_iter, and also
> struct gpiod_line_iter.
> They seem worse than the user performing a for-loop over the
> bulk indicies or chip offsets and getting each line themselves. They
> add a malloc overhead, in the case of gpiod_line_iter both a malloc and
> a calloc, as well as the corresponding frees.
>
> What benefit do they provide?
>
> Similarly gpiod_line_bulk_foreach_line().
>
> And I'm not sure about the utility of the struct gpiod_chip_iter either
> as it bails if opening any of the chips fails.  There a several reasons
> that could occur, e.g. permissions or unplugging, so you might want to
> leave it to the user to decide what to do.
>
> So I would prefer an iter that provides prospective chip names, or just
> a scandir() wrapper that returns an array of names.
>

I can think of a couple reasons to keep iterators in some form (while
of course addressing the issues you mentioned) but I'll start a
separate thread for discussion about changes in the API.

> Wrt rethinking the libgpiod API for v2, I'd like to either reduce the
> API to a minimal function set, for example stripping out the iters, or
> at least identify the minimal set that everything else is built on, and
> then look at how to rework those to expose the uAPI v2 features.
> e.g. given lines in a bulk now have to all be on the same chip, and given
> uAPI v2 supports per-line configs even within a bulk, the whole line and
> bulk lifecycle, as well as their relationship the chip and the line
> request, can be rethought.
>

True. I'd also consider removing the entire ctxless family of
functions and data structures. Throughout the entirety of libgpiod's
lifetime I've received a lot of emails from users with various
questions but nobody has ever asked me about the ctxless functions
which makes me think that nobody uses it. If so - there's no need to
keep it.

> I'd also like to see ALL libgpiod types become opaque.
>

I'm not sure this is desirable, not for event structs anyway. While
it's true that not exposing any structures to users is recommended in
C libraries, this would impose a significant malloc overhead when
reading events. While it's acceptable in Python bindings - there's no
way around allocating Python objects anyway - I think that in
low-level C interface reading events into a buffer provided by the
user is still preferable.

We could probably live with the config structure being opaque - though
for most cases I think just adding additional padding would be enough
to keep ABI compatible. Opaque config structure would mean a lot of
unnecessary churn for getters/setters.

Agreed for all other types though.

> But that is getting way outside the scope of this patch...
>

Yes, let's discuss this in a dedicated thread. I'd really like to get
most stuff right this time to be able to guarantee ABI and API
compatibility over all feature releases in v2.x series.

Bartosz