Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e2fe6351-f9ee-48eb-ad7f-280249f7f3f7@intel.com>
Date:   Fri, 8 Jul 2022 18:32:38 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.10.0
Subject: Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()
Content-Language: en-US
To:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        =?UTF-8?Q?P=c3=a9ter_Ujfalusi?= <peter.ujfalusi@linux.intel.com>
CC:     Andy Shevchenko <andy@kernel.org>, Mark Brown <broonie@kernel.org>,
        "ALSA Development Mailing List" <alsa-devel@alsa-project.org>,
        Takashi Iwai <tiwai@suse.com>,
        Jaroslav Kysela <perex@perex.cz>,
        <amadeuszx.slawinski@linux.intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        "Kai Vehmanen" <kai.vehmanen@linux.intel.com>,
        Bard Liao <yung-chuan.liao@linux.intel.com>
References: <20220707091301.1282291-1-cezary.rojewski@intel.com>
 <CAHp75VceKBoxXVPP4dRYb8LQqHMMDHFp6-E2iuZ-h2RTK8PWQQ@mail.gmail.com>
 <e0c7d254-ace3-625c-cc83-52ca0b45e9fc@intel.com>
 <CAHp75VckU2ZraLJ-frjWXjUu9pFW+-XmWgTbYqUXOUNAD-1HGA@mail.gmail.com>
 <6c8e4104-2239-a188-649d-585f059cabdd@intel.com>
 <YsgjdKEtE7pMDTnZ@smile.fi.intel.com>
 <a73b3ec0-5abb-ddfd-414b-b9807f05413e@linux.intel.com>
 <CAHp75Vd4D0KF7ik+aMOwv-+bofWja_tDe4YUmihQBF+RiHZTmA@mail.gmail.com>
From:   Cezary Rojewski <cezary.rojewski@intel.com>
In-Reply-To: <CAHp75Vd4D0KF7ik+aMOwv-+bofWja_tDe4YUmihQBF+RiHZTmA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QUh6Y05DblViKzE3Y3dXc1d6RG9BYmh6cFZnZ0RmWmRrV3AxU1BwZ3VzN1RB?=
 =?utf-8?B?ajJiemszUlM3cGxwaHZ0R2dYZVBVU3NlTjFEcDVJRHAzNi92cXdJK2F4eTUy?=
 =?utf-8?B?T1ltK3J6TFkrNGlrK3dYYldPNTRKWTVoY0lacDZEdmkyVU9tRGIxa2VKbWpX?=
 =?utf-8?B?RXdsNHBrcXg3aEM1dEt0b1h6WGNmSkxQOVNKam5yMHM3eE11aHkyZGM1OFR1?=
 =?utf-8?B?RzJxNTl3RXBiWlM2aGpSRzJuN0t0cklpdHpKYzRKRXhGRTlUOHlEUUxoZWt6?=
 =?utf-8?B?WUY2TC9DUTFaTGI4SWxyR09YWDhTU1N6Q3hWL1VodURETmRuZndMSFdaeUQ3?=
 =?utf-8?B?aGF2OTVBQnV3Y3hJa2RWVHdBVnA3MlZjdnM4YTRFOGFUZndtb2tzRnFWMFdK?=
 =?utf-8?B?VlFobnpzSGtDc2UwNndQVWkyOFgyV2NyVi8zbUlHSnNjVFA0VmVZblFqN3ZO?=
 =?utf-8?B?WDk0a1lNVy9RaXY5U200dE9odEh4OTVmWVJMOFd4dnptc0hyNjhiVFFqd0Q2?=
 =?utf-8?B?RkUyNGRMWlBDRGh4YUoweXJTSDkrY0RoQTFZeDN4Y0Rkc3czS01sQnY4c295?=
 =?utf-8?B?SGh0aUtDMWcrTXZ4TlcyV2JManovTHdac29jWkxNWEhHcjBsU3lyQ0ZzSGV2?=
 =?utf-8?B?MzhhT0J6clUwcWlCZCs3V3BWNlZhZXNyK3lrdVhiazhjaGg2ZkVZOUZGdW9z?=
 =?utf-8?B?b3FtMmloYUNiR1J3NmVwTmZjMmJwcHB5Vjlhd2RXZDJGaUJoOVlwSFQ5TkJP?=
 =?utf-8?B?aGdlMzVoZWdEc09kM05aVUt0M1Ayb05URDRQaHg0bUJDb2lYTjdyQ0VHMURt?=
 =?utf-8?B?cmwxNnV5TXlQRmY0VEQ0WWU1a0FXaDFpMGdtTEdrZ0JqNWh4VTdQWVBBb3JP?=
 =?utf-8?B?MEJ1ZGxIMWdpRkFLUDluUHVETU1VZHZvYUlBYmUyU1MvcHIzVm9SdHJZaG1s?=
 =?utf-8?B?eXdDRDRjK2xUZUZEWmRiNGEyeXFWdVROYUVHZFlkdnZ0eVJuV3RZc0RML0J5?=
 =?utf-8?B?SG9Pa1l6YkZCc3VMTDlZMG9JbDQ4bEp6SlhkM1BRanVBdUVHblJNbGhoMDZu?=
 =?utf-8?B?allaSmppQTdyay9jc3RsdFlyK3BUcnVaR0hUOHdwR0JkYzZCWVI5emZaMlFK?=
 =?utf-8?B?dXZoeUV3SU5rcFNLRURBb0U1cnNLTG52RmdaZkUzeVR0M1pmMWI0OHhRRHFR?=
 =?utf-8?B?Q0FqaEZmTFRIMkhWNTl6LzFDdWN0NjNVUURUeHlWUDRFR1hQOHJpRWZ2WS9S?=
 =?utf-8?B?c1MyUjArVWpOYXpjT1Y5dmloQ2JNajdCZSswbHd5THpKb2FzYTdZenByOG41?=
 =?utf-8?B?TEdoSTgzY2tzUnpsclI2TmtsdkppcVFjZzRMdXRYTFR2a2FKUHBiRmJ4Vmtj?=
 =?utf-8?B?RGlmVHM4SFBicGVMWG5Ja1g2eDdMSjgveUpDbXJTNlpUdjZ0eWN4M0UrK1FQ?=
 =?utf-8?B?UTlycDFIRUFhRUJoNFVmcEFLbmo4OXBOYmY0SGQySnJnQkxkMHRWRjNxUUNI?=
 =?utf-8?B?aTdDQ1NEZE1aTVpNRzAydThWMG9EbFl4MUFsL01FclRFZTZtekpXZklKM1dG?=
 =?utf-8?B?OERHVWQwWUNXbVVFeUpKYXp2N0R2VjJUMmsyWEZzTkFLTGZZYWtPNXhVa09Z?=
 =?utf-8?B?ekhpTHM5bHFQZnI4d1QrdCtPTkZlZnZtRUVma2ZCNUlUZW9rcllBQ2s0dEJp?=
 =?utf-8?B?d1QxcHlMcEhJcUU0WjFld2NJaTRoL2QzZ2dOa0hnaDRBdTZtU1h0cDEwSnBt?=
 =?utf-8?B?WmVtT2FZancvOGxWU1M5R3UzYXBYVTFqWnB4emZ0bEl0d1QyMUt3UXVrSysx?=
 =?utf-8?B?bEgvN3JVZVlERldiUXZLOHNPUWhqSC9FRXpoR0RUL3gvUk5pRjJST2ZKREhy?=
 =?utf-8?B?YldmcGJ2a1ZEWW9meFpOTUo1R0wybU9kMW1OSEF6VDFkQWlJQ2wxOHI2aE5L?=
 =?utf-8?B?Tk1SZ2ZpTy9nTFJrM1F2WmplTzlSSG8yTHJCblZDMGlYMzFrK0dJaG5TN0kr?=
 =?utf-8?B?RE5GeEFLN1N3U0RBUksweFJIZ3BadFk1Qy92dkJwcVZhSWt1dkVRNXVDOVBu?=
 =?utf-8?B?MzNaTjF4Rnh2Q3duSkwyeUZMTW85dndYN0hCYU1haVpUMGVHdFdKaDhyZEtj?=
 =?utf-8?B?RTRSaWhHZmtpTGtlRnh3Vkd3dzM4VmVERk8wTDNlclVCbm0wNHdqazd1d0RJ?=
 =?utf-8?B?cHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2c12ab39-dbc6-4caa-1213-08da60ff7dac
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6375.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jul 2022 16:32:46.9420
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KW1fkcRQHtok8x+cevK0pco6Q1K6sLJKvSynChjWoNNr3dx92WjPbZ1rGPnKiqq6+cL23PisakV15MWeplXH1fLjPQxPLN0jRTwMShDexhg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB6066
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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);