Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

[Cezary Rojewski <cezary.rojewski@xxxxxxxxx>]

On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> <peter.ujfalusi@xxxxxxxxxxxxxxx> wrote:

...

> > > It seems you are missing the (1). The code has checks for the case where you
> > > can do get number upfront, it would just require two passes, but it's nothing
> > > in comparison of heave realloc().
> > >
> > >    unsigned int *tokens;
> > >    char *p;
> > >    int num;
> > >
> > >    p = get_options(str, 0, &num);
> > >    if (num == 0)
> > >        // No numbers in the string!
> > >
> > >    tokens = kcalloc(num + 1, sizeof(*tokens), GFP_KERNEL);
> > >    if (!tokens)
> > >        return -ENOMEM;
> > >
> > >    p = get_oprions(str, num, &tokens);
> > >    if (*p)
> > >        // String was parsed only partially!
> > >        // assuming it's not a fatal error
> > >
> > >    return tokens;
>
> > This diff is tested and works:
>
> Thanks, Peter!
>
> But at least you can memove() to avoid second allocation.
> ideally to refactor that the result of get_options is consumed as is
> (it may be casted to struct tokens { int n; u32 v[]; })


A long shot, but what if we were to modify get_options() so it takes 
additional element-size parameter instead? Something like below - I've 
ignored get_range() though. Will re-visit if this option is viable.


diff --git a/lib/cmdline.c b/lib/cmdline.c
index 5546bf588780..272f892b71df 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -53,7 +53,7 @@ static int get_range(char **str, int *pint, int n)
  *     for the sake of simplification.
  */

-int get_option(char **str, int *pint)
+int get_num_option(char **str, void *pint, size_t nsize)
 {
        char *cur = *str;
        int value;
@@ -65,7 +65,7 @@ int get_option(char **str, int *pint)
        else
                value = simple_strtoull(cur, str, 0);
        if (pint)
-               *pint = value;
+               memcpy(pint, &value, min(nsize, sizeof(value)));
        if (cur == *str)
                return 0;
        if (**str == ',') {
@@ -77,6 +77,12 @@ int get_option(char **str, int *pint)

        return 1;
 }
+EXPORT_SYMBOL(get_num_option);
+
+int get_option(char **str, int *pint)
+{
+       return get_num_option(str, pint, sizeof(*pint));
+}
 EXPORT_SYMBOL(get_option);

 /**
@@ -104,15 +110,15 @@ EXPORT_SYMBOL(get_option);
  *     completely parseable).
  */

-char *get_options(const char *str, int nints, int *ints)
+char *get_num_options(const char *str, int nints, void *ints, size_t nsize)
 {
        bool validate = (nints == 0);
        int res, i = 1;

        while (i < nints || validate) {
-               int *pint = validate ? ints : ints + i;
+               int *pint = validate ? ints : ints + (i * nsize);

-               res = get_option((char **)&str, pint);
+               res = get_num_option((char **)&str, pint, nsize);
                if (res == 0)
                        break;
                if (res == 3) {
@@ -133,9 +139,17 @@ char *get_options(const char *str, int nints, int 
*ints)
                if (res == 1)
                        break;
        }
-       ints[0] = i - 1;
+       --i;
+       memcpy(ints, &i, min(nsize, sizeof(i)));
        return (char *)str;
 }
+EXPORT_SYMBOL(get_num_options);
+
+char *get_options(const char *str, int nints, int *ints)
+{
+       return get_num_options(str, nints, ints, sizeof(*ints));
+}
+
 EXPORT_SYMBOL(get_options);