Re: [PATCH] kvargs: fix device iterator match from arguments

[Olivier Matz <olivier.matz@6wind.com>]

Hi Xueming,

On Mon, Nov 22, 2021 at 02:12:50PM +0800, Xueming Li wrote:
> Device iterator RTE_DEV_FOREACH() failed to return devices from
> classifier like "class=vdpa", because matching name from empty kvargs
> returns no result. If device name not specified in kvargs, the function
> should iterate all devices.
> 
> This patch allows empty devargs or devargs without name specified.
> 
> Fixes: 6aebb942907d ("kvargs: add function to get from key and value")
> Cc: olivier.matz@6wind.com
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
> 21.11 specific bug, no copy to stable.org
> ---
>  drivers/bus/auxiliary/auxiliary_params.c | 3 +++
>  drivers/bus/vdev/vdev_params.c           | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> index a9c7853ed1d..6a6382961ea 100644
> --- a/drivers/bus/auxiliary/auxiliary_params.c
> +++ b/drivers/bus/auxiliary/auxiliary_params.c
> @@ -27,6 +27,9 @@ auxiliary_dev_match(const struct rte_device *dev,
>  	const struct rte_kvargs *kvlist = _kvlist;
>  	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
>  
> +	/* Iterate all devices if name not specified. */
> +	if (kvlist == NULL || rte_kvargs_get(kvlist, key) == NULL)
> +		return 0;
>  	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
>  		return -1;
>  
> diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> index 37d95395e7a..bab4c0d1d08 100644
> --- a/drivers/bus/vdev/vdev_params.c
> +++ b/drivers/bus/vdev/vdev_params.c
> @@ -29,6 +29,9 @@ vdev_dev_match(const struct rte_device *dev,
>  	const struct rte_kvargs *kvlist = _kvlist;
>  	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
>  
> +	/* Iterate all devices if name not specified. */
> +	if (kvlist == NULL || rte_kvargs_get(kvlist, key) == NULL)
> +		return 0;
>  	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
>  		return -1;
>  


Thank you for spotting and fixing this issue. The patch looks good to
me, but may I suggest an alternative that would avoid to browse the
kvlist twice? It is not yes tested, just for discussion. The idea
is to add an errno for error cases of rte_kvargs_get_with_value()
to identify the different cases.

  --- a/drivers/bus/auxiliary/auxiliary_params.c
  +++ b/drivers/bus/auxiliary/auxiliary_params.c
  @@ -27,7 +27,9 @@ auxiliary_dev_match(const struct rte_device *dev,
   	const struct rte_kvargs *kvlist = _kvlist;
   	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
   
  -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
  +	/* if kvlist is valid and contains the key, filter matching devices */
  +	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL &&
  +	    rte_errno == ENOENT)
   		return -1;
   
   	return 0;
  diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
  index 37d95395e7..0a5a8a9f58 100644
  --- a/drivers/bus/vdev/vdev_params.c
  +++ b/drivers/bus/vdev/vdev_params.c
  @@ -29,7 +29,9 @@ vdev_dev_match(const struct rte_device *dev,
   	const struct rte_kvargs *kvlist = _kvlist;
   	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
   
  -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
  +	/* if kvlist is valid and contains the key, filter matching devices */
  +	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL &&
  +	    rte_errno == ENOENT)
   		return -1;
   
   	return 0;
  diff --git a/lib/kvargs/rte_kvargs.c b/lib/kvargs/rte_kvargs.c
  index 11f624ef14..f1491715bf 100644
  --- a/lib/kvargs/rte_kvargs.c
  +++ b/lib/kvargs/rte_kvargs.c
  @@ -209,17 +209,28 @@ const char *
   rte_kvargs_get_with_value(const struct rte_kvargs *kvlist, const char *key,
   			  const char *value)
   {
  +	int key_found = 0;
   	unsigned int i;
   
  -	if (kvlist == NULL)
  +	if (kvlist == NULL) {
  +		rte_errno = EINVAL;
   		return NULL;
  +	}
  +
   	for (i = 0; i < kvlist->count; ++i) {
   		if (key != NULL && strcmp(kvlist->pairs[i].key, key) != 0)
   			continue;
  +		key_found = 1;
   		if (value != NULL && strcmp(kvlist->pairs[i].value, value) != 0)
   			continue;
   		return kvlist->pairs[i].value;
   	}
  +
  +	if (key_found)
  +		rte_errno = ENODEV;
  +	else
  +		rte_errno = ENOENT;
  +
   	return NULL;
   }
   
  diff --git a/lib/kvargs/rte_kvargs.h b/lib/kvargs/rte_kvargs.h
  index 359a9f5b09..3cb22ece48 100644
  --- a/lib/kvargs/rte_kvargs.h
  +++ b/lib/kvargs/rte_kvargs.h
  @@ -152,8 +152,12 @@ const char *rte_kvargs_get(const struct rte_kvargs *kvlist, const char *key);
    *   The matching value. If NULL, any value will match.
    *
    * @return
  - *   NULL if no key matches the input,
  - *   a value associated with a matching key otherwise.
  + *   The value associated with a matching key/value on success.
  + *   On error, return NULL and rte_errno is set:
  + *   - EINVAL - kvlist is NULL
  + *   - ENOENT - no matching key/value tuple, but the key matches with
  + *              a different value
  + *   - ENODEV - key is not found in the kvlist
    */
   __rte_experimental
   const char *rte_kvargs_get_with_value(const struct rte_kvargs *kvlist,


Let me know if it would work for you. I can submit a patch if you want.

I can add a unit test for kvargs, but do you know where we could add a
unit test for the dev iterate?

Thanks,
Olivier