Re: [libgpiod] Rethinking struct gpiod_line_bulk

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Oct 13, 2020 at 10:53 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>

[snip]

> >
> > struct gpiod_line_bulk {
> >     struct gpiod_chip *owner;
> >     unsigned int num_lines;
> >     uint64_t lines;
> > };
> >
> > And the 'lines' bitmap should actually refer to offsets at which the
> > owning chip stores the line pointers in its own 'lines' array - up to
> > 64 lines.
> >
> > But we'd still have to sanitize the values when adding lines to a bulk
> > object and probably check the return value. I'm wondering if there's a
> > better way to store group references to lines on the stack but I'm out
> > of ideas.
> >
>
> So you are proposing keeping the bulk of the bulk in the background and
> passing around a flyweight in its place.  Makes sense.
>

So this won't fly of course because a bitmap doesn't hold enough
information to reference an arbitrary number of lines in the chip in
any meaningful way.

I have another idea. I can live with struct gpiod_bulk being allocated
dynamically whenever users of the library use it - because it's quite
unlikely they'd do it all that often. What I'd like to avoid is
allocating a new bulk object whenever we want to package a single line
passed to e.g. gpiod_line_request() before we propagate it to
gpiod_line_request_bulk().

How about we define struct gpiod_line_bulk as:

struct gpiod_line_bulk {
    unsigned int num_lines;
    unsigned int max_num_lines;
    struct gpiod_line *lines[1];
};

And expose it in gpiod.h header?

That way we can still allocate it on the stack while using very little
memory - whenever packaging a single line - or we can allocate it
dynamically with the following interface:

struct gpiod_line_bulk *gpiod_line_bulk_new(unsigned int max_lines)
{
    struct gpiod_line_bulk *ret;

    if (max_lines < 1) {
        errno = EINVAL;
        return NULL;
    }

    ret = malloc(sizeof(struct gpiod_line_bulk) + (max_lines - 1) *
sizeof(struct gpiod_line *));
    if (!ret)
        return NULL;

    gpiod_line_bulk_init(max_lines);

    return ret;
}

Or we can even not expose it to users, make it completely opaque,
provide needed accessors and only allow internal users inside the
library to use the stack for single line bulks.

Bartosz