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

Simon Glass
2021-11-05T02:04:17Z

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