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

[fengchengwen]

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 at 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;
>>  			}
> 
> .
>