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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 11/6/23 15:29, fengchengwen wrote:
> 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@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.

I just want to highlight that it will not make this patch correct. So,

Nack

> 
>>
>>>    }
>>>      int
>>
>> .