Re: [PATCH] dpdk: remove redundant null check when parse kvargs

[David Marchand <david.marchand@redhat.com>]

Hello,

On Wed, Oct 30, 2024 at 5:00 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 36b06b3ac5..9366cb39b8 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -2484,19 +2484,19 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev)
>                         }
>
>                         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;
>                         }

Ok, it restores compatibility with some strange arguments passed by
user (and, on the principle, for this reason, it should be in a
separate commit with a Fixes: de89988365a7 ("kvargs: rework process
API")).

But on the other hand, this case does not make sense.

Those two helpers do nothing if there is no value.
Plus, those arguments are documented as taking a value:
                              ETH_TAP_MAC_ARG "=" ETH_TAP_MAC_ARG_FMT
" "
                              ETH_TAP_REMOTE_ARG "=<string>");

So in the end, the current logic looks more like an oversight in the
code, and the case "only key" should be rightfully rejected.

IOW: I would keep rte_kvargs_process() for them, but remove checks on
value == NULL in those two helpers.


Extract:
static int
set_mac_type(const char *key __rte_unused,
             const char *value,
             void *extra_args)
{
        struct rte_ether_addr *user_mac = extra_args;

        if (!value)
                return 0;
...
}

static int
set_remote_iface(const char *key __rte_unused,
                 const char *value,
                 void *extra_args)
{
        char *name = (char *)extra_args;

        if (value) {
...
        }

        return 0;
}


The rest of the patch looks correct to me, and I did not spot a missed update.
Thanks.


-- 
David Marchand