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

[Kent Gibson <warthog618@xxxxxxxxx>]

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?
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?

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.

Cheers,
Kent.