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

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-07-08 5:25 PM, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 2:34 PM Péter Ujfalusi
> <peter.ujfalusi@linux.intel.com> 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);