Re: [libgpiod][PATCH 04/16] treewide: unify gpiod_line_config/request_get_offsets() functions

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Jan 16, 2023 at 1:14 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Fri, Jan 13, 2023 at 10:51:58PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> >
> > We have two functions in the C API that allow users to retrieve a list
> > of offsets from objects: gpiod_line_request_get_offsets() and
> > gpiod_line_config_get_offsets(). Even though they serve pretty much the
> > same purpose, they have different signatures and one of them also
> > requires the user to free the memory allocated within the libgpiod
> > library with a non-libgpiod free() function.
> >
>
> They differ because they operate in different circumstances.
> Requests are immutable, wrt lines/offsets, while configs are not.
> More on this below.
>
> > Unify them: make them take the array in which to store offsets and the
> > size of this array. Make them return the number of offsets actually
> > stored in the array and make them impossible to fail. Change their names
> > to be more descriptive and in the case of line_config: add a new function
> > that allows users to get the number of configured offsets.
> >
>
> Not sure symmetry => beauty in this case.
>
> > Update the entire tree to use the new interfaces.
> >
> > For rust bindings: also unify the line config interface to return a map
> > of line settings like C++ bindings do instead of having a function to
> > get settings by offset. A map returned from a single call is easier to
> > iterate over with a for loop than using an integer and calling the
> > previous line_settings() method.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> <snip>
> > --- a/include/gpiod.h
> > +++ b/include/gpiod.h
> > @@ -780,19 +780,29 @@ struct gpiod_line_settings *
> >  gpiod_line_config_get_line_settings(struct gpiod_line_config *config,
> >                                   unsigned int offset);
> >
> > +/**
> > + * @brief Get the number of configured line offsets.
> > + * @param config Line config object.
> > + * @return Number of offsets for which line settings have been added.
> > + */
> > +size_t
> > +gpiod_line_config_get_num_configured_offsets(struct gpiod_line_config *config);
> > +
>               ^^^^^^         ^^^^^^^^^^
> Not a fan of the stuttering.
> What other kinds of lines are there in the config?

The user may not have an in-depth knowledge of the data model we use
and just wants to use the library. I think this name is much more
descriptive that way. It's not that long or repetetive, have you seen
udev_monitor_filter_add_match_subsystem_devtype() or
kmod_module_dependency_symbol_get_symbol()? :)

> Similarly requested lines in the request.
>
> >  /**
> >   * @brief Get configured offsets.
> >   * @param config Line config object.
> > - * @param num_offsets Pointer to a variable in which the number of line offsets
> > - *                    will be stored.
> > - * @param offsets Pointer to a pointer which will be set to point to an array
> > - *                containing the configured offsets. The array will be allocated
> > - *                using malloc() and must be freed using free().
> > - * @return 0 on success, -1 on failure.
> > + * @param offsets Array to store offsets.
> > + * @param max_offsets Number of offsets that can be stored in the offsets array.
> > + * @return Number of offsets stored in the offsets array.
> > + *
> > + * If max_offsets is lower than the number of lines actually requested (this
> > + * value can be retrieved using ::gpiod_line_config_get_num_configured_offsets),
> > + * then only up to max_lines offsets will be stored in offsets.
> >   */
> > -int gpiod_line_config_get_offsets(struct gpiod_line_config *config,
> > -                               size_t *num_offsets,
> > -                               unsigned int **offsets);
> > +size_t
> > +gpiod_line_config_get_configured_offsets(struct gpiod_line_config *config,
> > +                                      unsigned int *offsets,
> > +                                      size_t max_offsets);
> >
>
> So, to be sure that they have all the offsets, the user needs to call
> gpiod_line_config_get_num_configured_offsets() before every call to
> gpiod_line_config_get_configured_offsets()?
> Unless they can be sure no lines have been added subsequently?
>

Yes. Just like when you add config for new lines and need to be sure
it hasn't changed between that and the request. Perfectly normal for a
library that doesn't provide any thread-safety.

> Another way of looking at it is that the old API combines those two
> functions in one, hence the difference from the request equivalent.
> The old way you got them all, no matter what and no questions asked.
> That seems simpler to me, so I'm not sure this is a step forward.
>

I'd like to avoid having the library provide resources that are then
managed with functions from outside of the library (even if it's a
standard library function). That's bad practice and we should provide
our own function for freeing the allocated resource which in turn
makes the API more complicated than what this patch proposes. We still
need that *_get_num_*() variant.

Bart