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

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

On Tue, 2021-11-23 at 11:25 +0100, Olivier Matz wrote:
> 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.

Yes, the code walk the kvlist twice. An alternative complex code could
be this:

	if (kvlist == NULL)
		return 0;
	name = rte_kvargs_get(kvlist, key);
	if (name == NULL)
		/* Iterate all devices if name not specified. */
		return 0;
	if (strcmp(name, dev->name) != 0)
  		return -1;

> 
>   --- 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)

rte_errno == ENODEV? we should allow ENOENT - name not specified. 

>    		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)

Same, ENODEV? which means name specified but not match.

>    		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.


LGTM, let's use your patch.

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

Good idea, how about test_devargs.c?

> 
> Thanks,
> Olivier