[PATCH v5 2/5] net/sfc: use new API to parse kvargs

[fengchengwen]

Hi Andrew,

On 2023/11/6 18:28, Andrew Rybchenko wrote:
> On 11/6/23 10:31, Chengwen Feng wrote:
>> The sfc_kvargs_process() and sfc_efx_dev_class_get() function could
>> handle both key=value and only-key, so they should use
>> rte_kvargs_process_opt() instead of rte_kvargs_process() to parse.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen at huawei.com>
>> ---
>>   drivers/common/sfc_efx/sfc_efx.c | 4 ++--
>>   drivers/net/sfc/sfc_kvargs.c     | 2 +-
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/common/sfc_efx/sfc_efx.c b/drivers/common/sfc_efx/sfc_efx.c
>> index 2dc5545760..3ebac909f1 100644
>> --- a/drivers/common/sfc_efx/sfc_efx.c
>> +++ b/drivers/common/sfc_efx/sfc_efx.c
>> @@ -52,8 +52,8 @@ sfc_efx_dev_class_get(struct rte_devargs *devargs)
>>           return dev_class;
>>         if (rte_kvargs_count(kvargs, RTE_DEVARGS_KEY_CLASS) != 0) {
>> -        rte_kvargs_process(kvargs, RTE_DEVARGS_KEY_CLASS,
>> -                   sfc_efx_kvarg_dev_class_handler, &dev_class);
>> +        rte_kvargs_process_opt(kvargs, RTE_DEVARGS_KEY_CLASS,
>> +                       sfc_efx_kvarg_dev_class_handler, &dev_class);
> 
> LGTM from code point of view, but I'm not sure that I understand the
> idea behind handling NULL value in sfc_efx_kvarg_dev_class_handler().
> 
> Cc: Vijay
> 
>>       }
>>         rte_kvargs_free(kvargs);
>> diff --git a/drivers/net/sfc/sfc_kvargs.c b/drivers/net/sfc/sfc_kvargs.c
>> index 783cb43ae6..24bb896179 100644
>> --- a/drivers/net/sfc/sfc_kvargs.c
>> +++ b/drivers/net/sfc/sfc_kvargs.c
>> @@ -70,7 +70,7 @@ sfc_kvargs_process(struct sfc_adapter *sa, const char *key_match,
>>       if (sa->kvargs == NULL)
>>           return 0;
>>   -    return -rte_kvargs_process(sa->kvargs, key_match, handler, opaque_arg);
>> +    return -rte_kvargs_process_opt(sa->kvargs, key_match, handler, opaque_arg);
> 
> It looks wrong to me since many handlers do not handle NULL string gracefully. As I understand some handlers where fixed to avoid crash
> and correct fix would be to keep  rte_kvargs_process() and remove
> unnecessary checks for NULL string value.

The scope is large, I suggest creates a new patchset later which remove unnecessary checks for NULL string value.

> 
>>   }
>>     int
> 
> .