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 <fengcheng...@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;
>> }
>
> .
>
