diff mbox series

[v3,4/5] net/tap: use new API to parse kvargs

Message ID 20231103073811.13196-5-fengchengwen@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series fix segment fault when parse args |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

fengchengwen Nov. 3, 2023, 7:38 a.m. UTC 
  This driver could handles both key=value and only-key kvargs, so it
should use rte_kvargs_process_opt() instead of rte_kvargs_process() to
parse.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/tap/rte_eth_tap.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comments

Ferruh Yigit Nov. 3, 2023, 1:34 p.m. UTC | #1 
On 11/3/2023 7:38 AM, Chengwen Feng wrote:
> This driver could handles both key=value and only-key kvargs, so it
> should use rte_kvargs_process_opt() instead of rte_kvargs_process() to
> parse.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index b25a52655f..8b35de0a7a 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2342,7 +2342,7 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>  		kvlist = rte_kvargs_parse(params, valid_arguments);
>  		if (kvlist) {
>  			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
> -				ret = rte_kvargs_process(kvlist,
> +				ret = rte_kvargs_process_opt(kvlist,
>  					ETH_TAP_IFACE_ARG,
>  					&set_interface_name,
>  					tun_name);
> @@ -2546,28 +2546,28 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>  		kvlist = rte_kvargs_parse(params, valid_arguments);
>  		if (kvlist) {
>  			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
> -				ret = rte_kvargs_process(kvlist,
> -							 ETH_TAP_IFACE_ARG,
> -							 &set_interface_name,
> -							 tap_name);
> +				ret = rte_kvargs_process_opt(kvlist,
> +							     ETH_TAP_IFACE_ARG,
> +							     &set_interface_name,
> +							     tap_name);
>  				if (ret == -1)
>  					goto leave;
>  			}
>  
>  			if (rte_kvargs_count(kvlist, ETH_TAP_REMOTE_ARG) == 1) {
> -				ret = rte_kvargs_process(kvlist,
> -							 ETH_TAP_REMOTE_ARG,
> -							 &set_remote_iface,
> -							 remote_iface);
> +				ret = rte_kvargs_process_opt(kvlist,
> +							     ETH_TAP_REMOTE_ARG,
> +							     &set_remote_iface,
> +							     remote_iface);
>

As far as I can see, "remote" arg without value is not valid, but driver
handles this case as if "remote" arg is not provided at all. I think it
is reasonable to keep using 'rte_kvargs_process()', and fail if 'value'
is not provided.


>  				if (ret == -1)
>  					goto leave;
>  			}
>  
>  			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> -				ret = rte_kvargs_process(kvlist,
> -							 ETH_TAP_MAC_ARG,
> -							 &set_mac_type,
> -							 &user_mac);
> +				ret = rte_kvargs_process_opt(kvlist,
> +							     ETH_TAP_MAC_ARG,
> +							     &set_mac_type,
> +							     &user_mac);

same here, 'rte_kvargs_process()' can be used, there is no point to give
"mac" keyword without value, that is same as not providing "mac" keyword
at all, so this can fail to notify user either provide a mac or remove
the argument.

I think current logic is to handle "value==null" case, otherwise this is
not a valid usecase.

>  				if (ret == -1)
>  					goto leave;
>  			}
fengchengwen Nov. 5, 2023, 5:57 a.m. UTC | #2 
Hi Ferruh,

  Thanks for deepin, both fix in v4.

On 2023/11/3 21:34, Ferruh Yigit wrote:
> On 11/3/2023 7:38 AM, Chengwen Feng wrote:
>> This driver could handles both key=value and only-key kvargs, so it
>> should use rte_kvargs_process_opt() instead of rte_kvargs_process() to
>> parse.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> ---
>>  drivers/net/tap/rte_eth_tap.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index b25a52655f..8b35de0a7a 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -2342,7 +2342,7 @@ rte_pmd_tun_probe(struct rte_vdev_device *dev)
>>  		kvlist = rte_kvargs_parse(params, valid_arguments);
>>  		if (kvlist) {
>>  			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
>> -				ret = rte_kvargs_process(kvlist,
>> +				ret = rte_kvargs_process_opt(kvlist,
>>  					ETH_TAP_IFACE_ARG,
>>  					&set_interface_name,
>>  					tun_name);
>> @@ -2546,28 +2546,28 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>>  		kvlist = rte_kvargs_parse(params, valid_arguments);
>>  		if (kvlist) {
>>  			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
>> -				ret = rte_kvargs_process(kvlist,
>> -							 ETH_TAP_IFACE_ARG,
>> -							 &set_interface_name,
>> -							 tap_name);
>> +				ret = rte_kvargs_process_opt(kvlist,
>> +							     ETH_TAP_IFACE_ARG,
>> +							     &set_interface_name,
>> +							     tap_name);
>>  				if (ret == -1)
>>  					goto leave;
>>  			}
>>  
>>  			if (rte_kvargs_count(kvlist, ETH_TAP_REMOTE_ARG) == 1) {
>> -				ret = rte_kvargs_process(kvlist,
>> -							 ETH_TAP_REMOTE_ARG,
>> -							 &set_remote_iface,
>> -							 remote_iface);
>> +				ret = rte_kvargs_process_opt(kvlist,
>> +							     ETH_TAP_REMOTE_ARG,
>> +							     &set_remote_iface,
>> +							     remote_iface);
>>
> 
> As far as I can see, "remote" arg without value is not valid, but driver
> handles this case as if "remote" arg is not provided at all. I think it
> is reasonable to keep using 'rte_kvargs_process()', and fail if 'value'
> is not provided.
> 
> 
>>  				if (ret == -1)
>>  					goto leave;
>>  			}
>>  
>>  			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>> -				ret = rte_kvargs_process(kvlist,
>> -							 ETH_TAP_MAC_ARG,
>> -							 &set_mac_type,
>> -							 &user_mac);
>> +				ret = rte_kvargs_process_opt(kvlist,
>> +							     ETH_TAP_MAC_ARG,
>> +							     &set_mac_type,
>> +							     &user_mac);
> 
> same here, 'rte_kvargs_process()' can be used, there is no point to give
> "mac" keyword without value, that is same as not providing "mac" keyword
> at all, so this can fail to notify user either provide a mac or remove
> the argument.
> 
> I think current logic is to handle "value==null" case, otherwise this is
> not a valid usecase.
> 
>>  				if (ret == -1)
>>  					goto leave;
>>  			}
> 
> .
>
diff mbox series

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index b25a52655f..8b35de0a7a 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -2342,7 +2342,7 @@  rte_pmd_tun_probe(struct rte_vdev_device *dev)
 		kvlist = rte_kvargs_parse(params, valid_arguments);
 		if (kvlist) {
 			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
-				ret = rte_kvargs_process(kvlist,
+				ret = rte_kvargs_process_opt(kvlist,
 					ETH_TAP_IFACE_ARG,
 					&set_interface_name,
 					tun_name);
@@ -2546,28 +2546,28 @@  rte_pmd_tap_probe(struct rte_vdev_device *dev)
 		kvlist = rte_kvargs_parse(params, valid_arguments);
 		if (kvlist) {
 			if (rte_kvargs_count(kvlist, ETH_TAP_IFACE_ARG) == 1) {
-				ret = rte_kvargs_process(kvlist,
-							 ETH_TAP_IFACE_ARG,
-							 &set_interface_name,
-							 tap_name);
+				ret = rte_kvargs_process_opt(kvlist,
+							     ETH_TAP_IFACE_ARG,
+							     &set_interface_name,
+							     tap_name);
 				if (ret == -1)
 					goto leave;
 			}
 
 			if (rte_kvargs_count(kvlist, ETH_TAP_REMOTE_ARG) == 1) {
-				ret = rte_kvargs_process(kvlist,
-							 ETH_TAP_REMOTE_ARG,
-							 &set_remote_iface,
-							 remote_iface);
+				ret = rte_kvargs_process_opt(kvlist,
+							     ETH_TAP_REMOTE_ARG,
+							     &set_remote_iface,
+							     remote_iface);
 				if (ret == -1)
 					goto leave;
 			}
 
 			if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
-				ret = rte_kvargs_process(kvlist,
-							 ETH_TAP_MAC_ARG,
-							 &set_mac_type,
-							 &user_mac);
+				ret = rte_kvargs_process_opt(kvlist,
+							     ETH_TAP_MAC_ARG,
+							     &set_mac_type,
+							     &user_mac);
 				if (ret == -1)
 					goto leave;
 			}