diff mbox series

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

Message ID 20231106073125.55280-3-fengchengwen@huawei.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series fix segment fault when parse args |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

fengchengwen Nov. 6, 2023, 7:31 a.m. UTC 
  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(-)

Comments

Andrew Rybchenko Nov. 6, 2023, 10:28 a.m. UTC | #1 
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.

>   }
>   
>   int
fengchengwen Nov. 6, 2023, 12:29 p.m. UTC | #2 
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.

> 
>>   }
>>     int
> 
> .
Andrew Rybchenko Nov. 7, 2023, 7:18 a.m. UTC | #3 
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
>>
>> .
diff mbox series

Patch

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