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

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

On Tue, 2021-11-23 at 21:02 +0100, Olivier Matz wrote:
> Hello Xueming,
> 
> On Tue, Nov 23, 2021 at 12:49:32PM +0000, Xueming(Steven) Li wrote:
> > On Tue, 2021-11-23 at 13:31 +0100, Olivier Matz wrote:
> > > On Tue, Nov 23, 2021 at 11:25:22AM +0000, Xueming(Steven) Li wrote:
> > > > 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;
> > > 
> > > Maybe it is equivalent for common usages, but a difference is that
> > > rte_kvargs_get_with_value(kvlist, "key", "value2") also matches
> > > a kvlist="key=value1,key=value2" (i.e. the case where the key appears
> > > several times with different values).
> > > 
> > > 
> > > > > 
> > > > >   --- 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;
> > > 
> > > I think the patch is correct: the item is filtered only if there is no
> > > matching key/value tuple, but the key is found.
> > 
> > For "class=vdpa", name key not specified, all devices should be walked.
> > the kvargs is "" in such case.
> > 
> > > 
> > > If kvlist is NULL (rte_errno=EINVAL), or if the key does not exist
> > > (rte_errno=ENODEV), we return 0.
> > > 
> > > As a side note, there is an errno ENOKEY, which would be much better
> > > than ENODEV... but it seems it is not POSIX, so it may not exist on
> > > FreeBSD or Windows.
> > > 
> > > > >    
> > > > >    	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.
> > > 
> > > OK, I'll submit this patch then. Thanks for the feedback.
> 
> I'm trying my patch, but I run into an issue.
> Currently, EAL already depends on kvargs.
> If I add a reference to rte_errno from kvargs, it will require a
> new dependency to EAL... which is impossible.
> 
> Using errno instead of rte_errno is not usual in DPDK.
> Having a typedef for this is not very convenient either.
> 
> So thinking more about it, your solution looks better.
> 
> I have a draft of unit test, I'll send an update of your patch including
> it ASAP.

V2 sent, please check.
Unit test not critical, could be sent later IIUC.


> 
> > > 
> > > > > 
> > > > > 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, I'll have a look.
> > > 
> > > > > 
> > > > > Thanks,
> > > > > Olivier
> > > > 
> >