[v2,08/12] sysinfo: Add support for iterating string list

From: Marek Behún <marek.behun@nic.cz>

Add function
  sysinfo_get_str_list_max_len()
to determine length of the longest string in a string list, functions
  sysinfo_str_list_first() and
  sysinfo_str_list_next()
to support iteration over string list elements and macro
  for_each_sysinfo_str_list()
to make the iteration simple to use.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/sysinfo/sysinfo-uclass.c | 79 +++++++++++++++++++++++++
 include/sysinfo.h                | 99 ++++++++++++++++++++++++++++++++
 test/dm/sysinfo.c                | 35 +++++++++++
 3 files changed, 213 insertions(+)

Hi Marek,

On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> Add function
>   sysinfo_get_str_list_max_len()
> to determine length of the longest string in a string list, functions
>   sysinfo_str_list_first() and
>   sysinfo_str_list_next()
> to support iteration over string list elements and macro
>   for_each_sysinfo_str_list()
> to make the iteration simple to use.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  drivers/sysinfo/sysinfo-uclass.c | 79 +++++++++++++++++++++++++
>  include/sysinfo.h                | 99 ++++++++++++++++++++++++++++++++
>  test/dm/sysinfo.c                | 35 +++++++++++
>  3 files changed, 213 insertions(+)
>
> diff --git a/drivers/sysinfo/sysinfo-uclass.c b/drivers/sysinfo/sysinfo-uclass.c
> index d945f073c5..78035a95aa 100644
> --- a/drivers/sysinfo/sysinfo-uclass.c
> +++ b/drivers/sysinfo/sysinfo-uclass.c
> @@ -8,6 +8,7 @@
>
>  #include <common.h>
>  #include <dm.h>
> +#include <malloc.h>
>  #include <sysinfo.h>
>
>  struct sysinfo_priv {
> @@ -104,6 +105,84 @@ int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
>         return ops->get_str_list(dev, id, idx, size, val);
>  }
>
> +int sysinfo_get_str_list_max_len(struct udevice *dev, int id)
> +{
> +       int maxlen = 0;
> +       unsigned i;
> +
> +       /* first find out length of the longest string in the list */
> +       for (i = 0; ; ++i) {
> +               char dummy[1];
> +               int len;
> +
> +               len = sysinfo_get_str_list(dev, id, i, 0, dummy);
> +               if (len == -ERANGE)
> +                       break;
> +               else if (len < 0)
> +                       return len;
> +               else if (len > maxlen)
> +                       maxlen = len;
> +       }
> +
> +       return maxlen;
> +}
> +
> +struct sysinfo_str_list_iter {
> +       struct udevice *dev;
> +       int id;
> +       size_t size;
> +       unsigned idx;
> +       char value[];
> +};
> +
> +char *sysinfo_str_list_first(struct udevice *dev, int id, void *_iter)

Better to make iter a struct sysinfo_str_list_iter, I think and
require the caller to declare it:

sysinfo_str_list_iter iter;
char str[80]'

p = sysinfo_str_list_first(dev, i, &str, sizeof(str), &iter);
...

Do you need the iter?

If you want to support arbitratry length, I suppose that is OK?? But I
don't like allocating memory unless it is needed.

> +{
> +       struct sysinfo_str_list_iter *iter, **iterp = _iter;
> +       int maxlen, res;
> +
> +       maxlen = sysinfo_get_str_list_max_len(dev, id);
> +       if (maxlen < 0)
> +               return NULL;
> +
> +       iter = malloc(sizeof(struct sysinfo_str_list_iter) + maxlen + 1);
> +       if (!iter) {
> +               printf("No memory for sysinfo string list iterator\n");
> +               return NULL;
> +       }
> +
> +       iter->dev = dev;
> +       iter->id = id;
> +       iter->size = maxlen + 1;
> +       iter->idx = 0;
> +
> +       res = sysinfo_get_str_list(dev, id, 0, iter->size, iter->value);
> +       if (res < 0) {
> +               *iterp = NULL;
> +               free(iter);
> +               return NULL;
> +       }
> +
> +       *iterp = iter;
> +
> +       return iter->value;
> +}
> +
> +char *sysinfo_str_list_next(void *_iter)
> +{
> +       struct sysinfo_str_list_iter **iterp = _iter, *iter = *iterp;
> +       int res;
> +
> +       res = sysinfo_get_str_list(iter->dev, iter->id, ++iter->idx, iter->size,
> +                                  iter->value);
> +       if (res < 0) {
> +               *iterp = NULL;
> +               free(iter);
> +               return NULL;
> +       }
> +
> +       return iter->value;
> +}
> +
>  UCLASS_DRIVER(sysinfo) = {
>         .id             = UCLASS_SYSINFO,
>         .name           = "sysinfo",
> diff --git a/include/sysinfo.h b/include/sysinfo.h
> index 0d8a2d1676..d32bf3e808 100644
> --- a/include/sysinfo.h
> +++ b/include/sysinfo.h
> @@ -218,6 +218,61 @@ int sysinfo_get_str(struct udevice *dev, int id, size_t size, char *val);
>  int sysinfo_get_str_list(struct udevice *dev, int id, unsigned idx, size_t size,
>                          char *val);
>
> +/**
> + * sysinfo_get_str_list_max_len() - Get length of longest string in a string
> + *                                 list that describes hardware setup.
> + * @dev:       The sysinfo instance to gather the data.
> + * @id:                A unique identifier for the string list to read from.
> + *
> + * Return: Length (excluding the terminating NULL-byte) of the longest string in
> + *        the string list, or -ve on error.
> + */
> +int sysinfo_get_str_list_max_len(struct udevice *dev, int id);
> +
> +/**
> + * sysinfo_str_list_first() - Start iterating a string list.
> + * @dev:       The sysinfo instance to gather the data.
> + * @id:                A unique identifier for the string list to read from.
> + * @_iter:     Pointer where iterator data will be stored.
> + *
> + * Pass a reference to a void * pointer as @_iter, i.e.
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *
> + * The function will allocate space for the value. You need to iterate all
> + * elements with sysinfo_str_list_next() for the space to be freed, or free
> + * the pointer stored in @_iter, i.e.
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *     if (first)
> + *             free(iter);
> + *
> + * Return: First string in the string list, or NULL on error.
> + */
> +char *sysinfo_str_list_first(struct udevice *dev, int id, void *_iter);
> +
> +/**
> + * sysinfo_str_list_next() - Get next string in the string string list.
> + * @_iter:     Pointer to iterator, filled in by sysinfo_str_list_first().
> + *
> + * Pass a reference to a void * pointer as @_iter, i.e.
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *     next = sysinfo_str_list_next(&iter);
> + *
> + * All elements must be iterated until the function returns NULL for the
> + * resources allocated for the iteration to be freed, or pointer stored in
> + * @_iter must be freed, i.e.:
> + *     void *iter;
> + *     first = sysinfo_str_list_first(dev, id, &iter);
> + *     next = sysinfo_str_list_next(&iter);
> + *     if (next)
> + *             free(iter);
> + *
> + * Return: Next string in the string list, NULL on end of list or NULL on error.
> + */
> +char *sysinfo_str_list_next(void *_iter);
> +
>  /**
>   * sysinfo_get() - Return the sysinfo device for the sysinfo in question.
>   * @devp: Pointer to structure to receive the sysinfo device.
> @@ -279,6 +334,22 @@ static inline int sysinfo_get_str_list(struct udevice *dev, int id,
>         return -ENOSYS;
>  }
>
> +static inline int sysinfo_get_str_list_max_len(struct udevice *dev, int id)
> +{
> +       return -ENOSYS;
> +}
> +
> +static inline char *sysinfo_str_list_first(struct udevice *dev, int id,
> +                                          void *_iter)
> +{
> +       return NULL;
> +}
> +
> +static inline char *sysinfo_str_list_next(void *_iter)
> +{
> +       return NULL;
> +}
> +
>  static inline int sysinfo_get(struct udevice **devp)
>  {
>         return -ENOSYS;
> @@ -291,4 +362,32 @@ static inline int sysinfo_get_fit_loadable(struct udevice *dev, int index,
>  }
>
>  #endif
> +
> +/**
> + * for_each_sysinfo_str_list - Iterate a sysinfo string list
> + * @dev:       The sysinfo instance to gather the data.
> + * @id:                A unique identifier for the string list to read from.
> + * @val:       String pointer for the value.
> + * @iter:      Pointer where iteration data are stored.
> + *
> + * Important: all elements of the list must be iterated for the iterator
> + * resources to be freed automatically. If you need to break from the for cycle,
> + * you need to free the iterator.
> + *
> + * Example:
> + *     char *value;
> + *     void *iter;
> + *     for_each_sysinfo_str_list(dev, MY_STR_LIST, value, iter) {
> + *             printf("Value: %s\n", value);
> + *             if (!strcmp(value, "needle")) {
> + *                     free(iter);
> + *                     break;
> + *             }
> + *     }
> + */
> +#define for_each_sysinfo_str_list(dev, id, val, iter)                  \
> +       for ((val) = sysinfo_str_list_first((dev), (id), &(iter));      \
> +            (val);                                                     \
> +            (val) = sysinfo_str_list_next(&(iter)))
> +
>  #endif
> diff --git a/test/dm/sysinfo.c b/test/dm/sysinfo.c
> index a6b246f2df..e9b70d8e1a 100644
> --- a/test/dm/sysinfo.c
> +++ b/test/dm/sysinfo.c
> @@ -75,3 +75,38 @@ static int dm_test_sysinfo(struct unit_test_state *uts)
>  }
>
>  DM_TEST(dm_test_sysinfo, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> +
> +static int dm_test_sysinfo_str_list_iter(struct unit_test_state *uts)
> +{
> +       struct udevice *sysinfo;
> +       char *value;
> +       void *iter;
> +       int idx;
> +
> +       ut_assertok(sysinfo_get(&sysinfo));
> +       ut_assert(sysinfo);
> +
> +       sysinfo_detect(sysinfo);
> +
> +       idx = 0;
> +       for_each_sysinfo_str_list(sysinfo, STR_VACATIONSPOT, value, iter) {
> +               switch (idx) {
> +               case 0:
> +                       ut_asserteq_str(value, "R'lyeh");
> +                       break;
> +               case 2:
> +                       ut_asserteq_str(value, "Plateau of Leng");
> +                       break;
> +               case 3:
> +                       ut_asserteq_str(value, "Carcosa");
> +                       break;
> +               }
> +               ++idx;
> +       }
> +
> +       ut_assert(NULL == iter);
> +
> +       return 0;
> +}
> +
> +DM_TEST(dm_test_sysinfo_str_list_iter, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> --
> 2.32.0
>

Regards,
Simon

On Thu, 4 Nov 2021 20:02:29 -0600
Simon Glass <sjg@chromium.org> wrote:

> Better to make iter a struct sysinfo_str_list_iter, I think and
> require the caller to declare it:
> 
> sysinfo_str_list_iter iter;
> char str[80]'
> 
> p = sysinfo_str_list_first(dev, i, &str, sizeof(str), &iter);
> ...
> 
> Do you need the iter?
> 
> If you want to support arbitratry length, I suppose that is OK?? But I
> don't like allocating memory unless it is needed.

Well if I am iterating through default environment variables
overwrites, they can be basically up to ENV_SIZE long. There may be
some long commands stored there.

Another solution would be to redesign sysinfo_get_str() and introduce
sysinfo_get_str_list() so that they won't fill a buffer given by user,
but instead have their own buffer in implementation and return const
char * pointer.

Marek

Hi Marek,

On Fri, 5 Nov 2021 at 05:24, Marek Behún <kabel@kernel.org> wrote:
>
> On Thu, 4 Nov 2021 20:02:29 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
> > Better to make iter a struct sysinfo_str_list_iter, I think and
> > require the caller to declare it:
> >
> > sysinfo_str_list_iter iter;
> > char str[80]'
> >
> > p = sysinfo_str_list_first(dev, i, &str, sizeof(str), &iter);
> > ...
> >
> > Do you need the iter?
> >
> > If you want to support arbitratry length, I suppose that is OK?? But I
> > don't like allocating memory unless it is needed.
>
> Well if I am iterating through default environment variables
> overwrites, they can be basically up to ENV_SIZE long. There may be
> some long commands stored there.

OK.

>
> Another solution would be to redesign sysinfo_get_str() and introduce
> sysinfo_get_str_list() so that they won't fill a buffer given by user,
> but instead have their own buffer in implementation and return const
> char * pointer.

Yes that was a design idea at the start...but I think at present I
like the current API. I just didn't understand your intent properly.

My new thoughts:
- pass in the iter so malloc() is not needed there (change str to a char *)
- return an int from your iterator functions, so you can tell when you
run out of memory and need to die
- comment struct sysinfo_str_list_iter
- use log_debug() instead of printf(), on error

Also I wonder about this:
- get the caller to provide the str buffer, and maxsize, so malloc()
is not needed

I can see the advantage of allocating though. I wonder if you might
keep track of the current buffer length and do a realloc() if it is
too small, each time? Scanning through to find the max length might be
slower? Some drivers may read from an I2C device, for example.

Regards,
Simon