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

[Simon Glass]

Hi Marek,

On Fri, 5 Nov 2021 at 05:24, Marek Behún <kabel at kernel.org> wrote:
>
> On Thu, 4 Nov 2021 20:02:29 -0600
> Simon Glass <sjg at 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