[PATCH] kvargs: fix device iterator match from arguments

[PATCH] kvargs: fix device iterator match from arguments

[Xueming Li]

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;
 
-- 
2.33.0

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

[Olivier Matz]

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

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

[Xueming(Steven) Li]

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

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

[Olivier Matz]

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.

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

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

[Xueming(Steven) Li]

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

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

[Olivier Matz]

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.

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

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

[Xueming(Steven) Li]

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

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

[Xueming Li]

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 | 14 +++++++++-----
 drivers/bus/vdev/vdev_params.c           | 14 +++++++++-----
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
index a9c7853ed1d..9cbc1f7c777 100644
--- a/drivers/bus/auxiliary/auxiliary_params.c
+++ b/drivers/bus/auxiliary/auxiliary_params.c
@@ -26,11 +26,15 @@ 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)
-		return -1;
-
-	return 0;
+	const char *name;
+
+	if (kvlist == NULL)
+		return 0;
+	name = rte_kvargs_get(kvlist, key);
+	if (name == NULL)
+		/* Iterate all devices if name not specified. */
+		return 0;
+	return strcmp(name, dev->name);
 }
 
 void *
diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
index 37d95395e7a..b4baecb7c0d 100644
--- a/drivers/bus/vdev/vdev_params.c
+++ b/drivers/bus/vdev/vdev_params.c
@@ -28,11 +28,15 @@ 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)
-		return -1;
-
-	return 0;
+	const char *name;
+
+	if (kvlist == NULL)
+		return 0;
+	name = rte_kvargs_get(kvlist, key);
+	if (name == NULL)
+		/* Iterate all devices if name not specified. */
+		return 0;
+	return strcmp(name, dev->name);
 }
 
 void *
-- 
2.34.0

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

[Olivier Matz]

Hi Xueming,

On Wed, Nov 24, 2021 at 06:17:40PM +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>

I just sent a v3 with the unit test.

In case there is an issue with it, your v2 looks good to me:
Acked-by: Olivier Matz <olivier.matz@6wind.com>

As a side note, I was wondering if we should keep
rte_kvargs_get_with_value() in kvargs api, since there
is no more user. I think there it can still be useful, let's
keep it for now.

Thanks!
Olivier

> ---
> 21.11 specific bug, no copy to stable.org
> ---
>  drivers/bus/auxiliary/auxiliary_params.c | 14 +++++++++-----
>  drivers/bus/vdev/vdev_params.c           | 14 +++++++++-----
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> index a9c7853ed1d..9cbc1f7c777 100644
> --- a/drivers/bus/auxiliary/auxiliary_params.c
> +++ b/drivers/bus/auxiliary/auxiliary_params.c
> @@ -26,11 +26,15 @@ 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)
> -		return -1;
> -
> -	return 0;
> +	const char *name;
> +
> +	if (kvlist == NULL)
> +		return 0;
> +	name = rte_kvargs_get(kvlist, key);
> +	if (name == NULL)
> +		/* Iterate all devices if name not specified. */
> +		return 0;
> +	return strcmp(name, dev->name);
>  }
>  
>  void *
> diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> index 37d95395e7a..b4baecb7c0d 100644
> --- a/drivers/bus/vdev/vdev_params.c
> +++ b/drivers/bus/vdev/vdev_params.c
> @@ -28,11 +28,15 @@ 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)
> -		return -1;
> -
> -	return 0;
> +	const char *name;
> +
> +	if (kvlist == NULL)
> +		return 0;
> +	name = rte_kvargs_get(kvlist, key);
> +	if (name == NULL)
> +		/* Iterate all devices if name not specified. */
> +		return 0;
> +	return strcmp(name, dev->name);
>  }
>  
>  void *
> -- 
> 2.34.0
> 

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

[Xueming(Steven) Li]

On Wed, 2021-11-24 at 12:07 +0100, Olivier Matz wrote:
> Hi Xueming,
> 
> On Wed, Nov 24, 2021 at 06:17:40PM +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>
> 
> I just sent a v3 with the unit test.
> 
> In case there is an issue with it, your v2 looks good to me:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> As a side note, I was wondering if we should keep
> rte_kvargs_get_with_value() in kvargs api, since there
> is no more user. I think there it can still be useful, let's
> keep it for now.

Agree, nice to keep.

> 
> Thanks!
> Olivier
> 
> > ---
> > 21.11 specific bug, no copy to stable.org
> > ---
> >  drivers/bus/auxiliary/auxiliary_params.c | 14 +++++++++-----
> >  drivers/bus/vdev/vdev_params.c           | 14 +++++++++-----
> >  2 files changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> > index a9c7853ed1d..9cbc1f7c777 100644
> > --- a/drivers/bus/auxiliary/auxiliary_params.c
> > +++ b/drivers/bus/auxiliary/auxiliary_params.c
> > @@ -26,11 +26,15 @@ 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)
> > -		return -1;
> > -
> > -	return 0;
> > +	const char *name;
> > +
> > +	if (kvlist == NULL)
> > +		return 0;
> > +	name = rte_kvargs_get(kvlist, key);
> > +	if (name == NULL)
> > +		/* Iterate all devices if name not specified. */
> > +		return 0;
> > +	return strcmp(name, dev->name);
> >  }
> >  
> >  void *
> > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> > index 37d95395e7a..b4baecb7c0d 100644
> > --- a/drivers/bus/vdev/vdev_params.c
> > +++ b/drivers/bus/vdev/vdev_params.c
> > @@ -28,11 +28,15 @@ 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)
> > -		return -1;
> > -
> > -	return 0;
> > +	const char *name;
> > +
> > +	if (kvlist == NULL)
> > +		return 0;
> > +	name = rte_kvargs_get(kvlist, key);
> > +	if (name == NULL)
> > +		/* Iterate all devices if name not specified. */
> > +		return 0;
> > +	return strcmp(name, dev->name);
> >  }
> >  
> >  void *
> > -- 
> > 2.34.0
> > 

[PATCH v3] bus: fix device iterator match from arguments

[Olivier Matz]

From: Xueming Li <xuemingl@nvidia.com>

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

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
bug is specific to 21.11, no need to cc stable@dpdk.org
---
 app/test/meson.build                     |   3 +
 app/test/test_vdev.c                     | 168 +++++++++++++++++++++++
 drivers/bus/auxiliary/auxiliary_params.c |   9 +-
 drivers/bus/vdev/vdev_params.c           |   9 +-
 4 files changed, 187 insertions(+), 2 deletions(-)
 create mode 100644 app/test/test_vdev.c

diff --git a/app/test/meson.build b/app/test/meson.build
index 961bebc5cb..19009da52e 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -153,6 +153,7 @@ test_sources = files(
         'test_trace.c',
         'test_trace_register.c',
         'test_trace_perf.c',
+        'test_vdev.c',
         'test_version.c',
         'virtual_pmd.c',
 )
@@ -177,6 +178,7 @@ test_deps = [
         'ipsec',
         'lpm',
         'member',
+        'net_null',
         'node',
         'pipeline',
         'port',
@@ -283,6 +285,7 @@ fast_tests = [
         ['service_autotest', true],
         ['thash_autotest', true],
         ['trace_autotest', true],
+        ['vdev_autotest', true],
 ]
 
 # Tests known to have issues or which don't belong in other tests lists.
diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
new file mode 100644
index 0000000000..720722c363
--- /dev/null
+++ b/app/test/test_vdev.c
@@ -0,0 +1,168 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 6WIND S.A.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_kvargs.h>
+#include <rte_bus_vdev.h>
+
+#include "test.h"
+
+#define TEST_VDEV_KEY_NAME "name"
+
+static const char * const valid_keys[] = {
+	TEST_VDEV_KEY_NAME,
+	NULL,
+};
+
+static int
+cmp_dev_name(const struct rte_device *dev, const void *name)
+{
+	return strcmp(dev->name, name);
+}
+
+static int
+cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
+{
+	const struct rte_kvargs *kvlist = _kvlist;
+	const char *key = TEST_VDEV_KEY_NAME;
+	const char *name;
+
+	/* no kvlist arg, all devices match */
+	if (kvlist == NULL)
+		return 0;
+
+	/* if key is present in kvlist and does not match, filter device */
+	name = rte_kvargs_get(kvlist, key);
+	if (name != NULL && strcmp(name, dev->name))
+		return -1;
+
+	return 0;
+}
+
+static struct rte_device *
+get_matching_vdev(const char *match_str)
+{
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+	struct rte_kvargs *kvargs = NULL;
+	struct rte_device *dev;
+
+	if (match_str != NULL) {
+		kvargs = rte_kvargs_parse(match_str, valid_keys);
+		if (kvargs == NULL) {
+			printf("Failed to parse match string\n");
+			return NULL;
+		}
+	}
+
+	dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs);
+	rte_kvargs_free(kvargs);
+
+	return dev;
+}
+
+static int
+test_vdev_bus(void)
+{
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+	struct rte_dev_iterator dev_iter = { 0 };
+	struct rte_device *dev, *dev0, *dev1;
+
+	/* not supported */
+	if (vdev_bus == NULL)
+		return 0;
+
+	/* create first vdev */
+	if (rte_vdev_init("net_null_test0", "") < 0) {
+		printf("Failed to create vdev net_null_test0\n");
+		goto fail;
+	}
+	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
+	if (dev0 == NULL) {
+		printf("Cannot find net_null_test0 vdev\n");
+		goto fail;
+	}
+
+	/* create second vdev */
+	if (rte_vdev_init("net_null_test1", "") < 0) {
+		printf("Failed to create vdev net_null_test1\n");
+		goto fail;
+	}
+	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
+	if (dev1 == NULL) {
+		printf("Cannot find net_null_test1 vdev\n");
+		goto fail;
+	}
+
+	/* try to match vdevs */
+	dev = get_matching_vdev("name=net_null_test0");
+	if (dev != dev0) {
+		printf("Cannot match net_null_test0 vdev\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev("name=net_null_test1");
+	if (dev != dev1) {
+		printf("Cannot match net_null_test1 vdev\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev("name=unexistant");
+	if (dev != NULL) {
+		printf("Unexistant vdev should not match\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev("");
+	if (dev == NULL || dev == dev1) {
+		printf("Cannot match any vdev with empty match string\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev(NULL);
+	if (dev == NULL || dev == dev1) {
+		printf("Cannot match any vdev with NULL match string\n");
+		goto fail;
+	}
+
+	/* iterate all vdevs, and ensure we find vdev0 and vdev1 */
+	RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) {
+		if (dev == dev0)
+			dev0 = NULL;
+		else if (dev == dev1)
+			dev1 = NULL;
+	}
+	if (dev0 != NULL) {
+		printf("dev0 was not iterated\n");
+		goto fail;
+	}
+	if (dev1 != NULL) {
+		printf("dev1 was not iterated\n");
+		goto fail;
+	}
+
+	rte_vdev_uninit("net_null_test0");
+	rte_vdev_uninit("net_null_test1");
+
+	return 0;
+
+fail:
+	rte_vdev_uninit("net_null_test0");
+	rte_vdev_uninit("net_null_test1");
+	return -1;
+}
+
+static int
+test_vdev(void)
+{
+	printf("== test vdev bus ==\n");
+	if (test_vdev_bus() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(vdev_autotest, test_vdev);
diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
index 8dd8813611..9c08ccdd1b 100644
--- a/drivers/bus/auxiliary/auxiliary_params.c
+++ b/drivers/bus/auxiliary/auxiliary_params.c
@@ -28,8 +28,15 @@ auxiliary_dev_match(const struct rte_device *dev,
 {
 	const struct rte_kvargs *kvlist = _kvlist;
 	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
+	const char *name;
 
-	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
+	/* no kvlist arg, all devices match */
+	if (kvlist == NULL)
+		return 0;
+
+	/* if key is present in kvlist and does not match, filter device */
+	name = rte_kvargs_get(kvlist, key);
+	if (name != NULL && strcmp(name, dev->name))
 		return -1;
 
 	return 0;
diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
index 37d95395e7..3969faf16d 100644
--- a/drivers/bus/vdev/vdev_params.c
+++ b/drivers/bus/vdev/vdev_params.c
@@ -28,8 +28,15 @@ vdev_dev_match(const struct rte_device *dev,
 {
 	const struct rte_kvargs *kvlist = _kvlist;
 	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
+	const char *name;
 
-	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
+	/* no kvlist arg, all devices match */
+	if (kvlist == NULL)
+		return 0;
+
+	/* if key is present in kvlist and does not match, filter device */
+	name = rte_kvargs_get(kvlist, key);
+	if (name != NULL && strcmp(name, dev->name))
 		return -1;
 
 	return 0;
-- 
2.30.2

Re: [PATCH v3] bus: fix device iterator match from arguments

[Xueming(Steven) Li]

On Wed, 2021-11-24 at 12:02 +0100, Olivier Matz wrote:
> From: Xueming Li <xuemingl@nvidia.com>
> 
> 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")
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
> bug is specific to 21.11, no need to cc stable@dpdk.org
> ---
>  app/test/meson.build                     |   3 +
>  app/test/test_vdev.c                     | 168 +++++++++++++++++++++++
>  drivers/bus/auxiliary/auxiliary_params.c |   9 +-
>  drivers/bus/vdev/vdev_params.c           |   9 +-
>  4 files changed, 187 insertions(+), 2 deletions(-)
>  create mode 100644 app/test/test_vdev.c
> 
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 961bebc5cb..19009da52e 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -153,6 +153,7 @@ test_sources = files(
>          'test_trace.c',
>          'test_trace_register.c',
>          'test_trace_perf.c',
> +        'test_vdev.c',
>          'test_version.c',
>          'virtual_pmd.c',
>  )
> @@ -177,6 +178,7 @@ test_deps = [
>          'ipsec',
>          'lpm',
>          'member',
> +        'net_null',

From my experience, CI will fail if build not configured with net_null,
even we add it here. Let's monitor CI result. 

>          'node',
>          'pipeline',
>          'port',
> @@ -283,6 +285,7 @@ fast_tests = [
>          ['service_autotest', true],
>          ['thash_autotest', true],
>          ['trace_autotest', true],
> +        ['vdev_autotest', true],
>  ]
>  
>  # Tests known to have issues or which don't belong in other tests lists.
> diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
> new file mode 100644
> index 0000000000..720722c363
> --- /dev/null
> +++ b/app/test/test_vdev.c
> @@ -0,0 +1,168 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 6WIND S.A.
> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include <rte_common.h>
> +#include <rte_kvargs.h>
> +#include <rte_bus_vdev.h>
> +
> +#include "test.h"
> +
> +#define TEST_VDEV_KEY_NAME "name"
> +
> +static const char * const valid_keys[] = {
> +	TEST_VDEV_KEY_NAME,
> +	NULL,
> +};
> +
> +static int
> +cmp_dev_name(const struct rte_device *dev, const void *name)
> +{
> +	return strcmp(dev->name, name);
> +}
> +
> +static int
> +cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
> +{
> +	const struct rte_kvargs *kvlist = _kvlist;
> +	const char *key = TEST_VDEV_KEY_NAME;
> +	const char *name;
> +
> +	/* no kvlist arg, all devices match */
> +	if (kvlist == NULL)
> +		return 0;
> +
> +	/* if key is present in kvlist and does not match, filter device */
> +	name = rte_kvargs_get(kvlist, key);
> +	if (name != NULL && strcmp(name, dev->name))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static struct rte_device *
> +get_matching_vdev(const char *match_str)
> +{
> +	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
> +	struct rte_kvargs *kvargs = NULL;
> +	struct rte_device *dev;
> +
> +	if (match_str != NULL) {
> +		kvargs = rte_kvargs_parse(match_str, valid_keys);
> +		if (kvargs == NULL) {
> +			printf("Failed to parse match string\n");
> +			return NULL;
> +		}
> +	}
> +
> +	dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs);
> +	rte_kvargs_free(kvargs);
> +
> +	return dev;
> +}
> +
> +static int
> +test_vdev_bus(void)
> +{
> +	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
> +	struct rte_dev_iterator dev_iter = { 0 };
> +	struct rte_device *dev, *dev0, *dev1;
> +
> +	/* not supported */
> +	if (vdev_bus == NULL)
> +		return 0;
> +
> +	/* create first vdev */
> +	if (rte_vdev_init("net_null_test0", "") < 0) {
> +		printf("Failed to create vdev net_null_test0\n");
> +		goto fail;
> +	}
> +	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
> +	if (dev0 == NULL) {
> +		printf("Cannot find net_null_test0 vdev\n");
> +		goto fail;
> +	}
> +
> +	/* create second vdev */
> +	if (rte_vdev_init("net_null_test1", "") < 0) {
> +		printf("Failed to create vdev net_null_test1\n");
> +		goto fail;
> +	}
> +	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
> +	if (dev1 == NULL) {
> +		printf("Cannot find net_null_test1 vdev\n");
> +		goto fail;
> +	}
> +
> +	/* try to match vdevs */
> +	dev = get_matching_vdev("name=net_null_test0");
> +	if (dev != dev0) {
> +		printf("Cannot match net_null_test0 vdev\n");
> +		goto fail;
> +	}
> +
> +	dev = get_matching_vdev("name=net_null_test1");
> +	if (dev != dev1) {
> +		printf("Cannot match net_null_test1 vdev\n");
> +		goto fail;
> +	}
> +
> +	dev = get_matching_vdev("name=unexistant");
> +	if (dev != NULL) {
> +		printf("Unexistant vdev should not match\n");
> +		goto fail;
> +	}
> +
> +	dev = get_matching_vdev("");
> +	if (dev == NULL || dev == dev1) {
> +		printf("Cannot match any vdev with empty match string\n");
> +		goto fail;
> +	}
> +
> +	dev = get_matching_vdev(NULL);
> +	if (dev == NULL || dev == dev1) {
> +		printf("Cannot match any vdev with NULL match string\n");
> +		goto fail;
> +	}
> +
> +	/* iterate all vdevs, and ensure we find vdev0 and vdev1 */
> +	RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) {
> +		if (dev == dev0)
> +			dev0 = NULL;
> +		else if (dev == dev1)
> +			dev1 = NULL;
> +	}
> +	if (dev0 != NULL) {
> +		printf("dev0 was not iterated\n");
> +		goto fail;
> +	}
> +	if (dev1 != NULL) {
> +		printf("dev1 was not iterated\n");
> +		goto fail;
> +	}
> +
> +	rte_vdev_uninit("net_null_test0");
> +	rte_vdev_uninit("net_null_test1");
> +
> +	return 0;
> +
> +fail:
> +	rte_vdev_uninit("net_null_test0");
> +	rte_vdev_uninit("net_null_test1");
> +	return -1;
> +}
> +
> +static int
> +test_vdev(void)
> +{
> +	printf("== test vdev bus ==\n");
> +	if (test_vdev_bus() < 0)
> +		return -1;
> +	return 0;
> +}
> +
> +REGISTER_TEST_COMMAND(vdev_autotest, test_vdev);
> diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> index 8dd8813611..9c08ccdd1b 100644
> --- a/drivers/bus/auxiliary/auxiliary_params.c
> +++ b/drivers/bus/auxiliary/auxiliary_params.c
> @@ -28,8 +28,15 @@ auxiliary_dev_match(const struct rte_device *dev,
>  {
>  	const struct rte_kvargs *kvlist = _kvlist;
>  	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
> +	const char *name;
>  
> -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
> +	/* no kvlist arg, all devices match */
> +	if (kvlist == NULL)
> +		return 0;
> +
> +	/* if key is present in kvlist and does not match, filter device */
> +	name = rte_kvargs_get(kvlist, key);
> +	if (name != NULL && strcmp(name, dev->name))
>  		return -1;
>  
>  	return 0;
> diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> index 37d95395e7..3969faf16d 100644
> --- a/drivers/bus/vdev/vdev_params.c
> +++ b/drivers/bus/vdev/vdev_params.c
> @@ -28,8 +28,15 @@ vdev_dev_match(const struct rte_device *dev,
>  {
>  	const struct rte_kvargs *kvlist = _kvlist;
>  	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
> +	const char *name;
>  
> -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
> +	/* no kvlist arg, all devices match */
> +	if (kvlist == NULL)
> +		return 0;
> +
> +	/* if key is present in kvlist and does not match, filter device */
> +	name = rte_kvargs_get(kvlist, key);
> +	if (name != NULL && strcmp(name, dev->name))
>  		return -1;
>  
>  	return 0;

Nice unit test!

Reviewed-by: Xueming Li <xuemingl@nvidia.com>

Re: [PATCH v3] bus: fix device iterator match from arguments

[Xueming(Steven) Li]

On Wed, 2021-11-24 at 19:30 +0800, Xueming Li wrote:
> On Wed, 2021-11-24 at 12:02 +0100, Olivier Matz wrote:
> > From: Xueming Li <xuemingl@nvidia.com>
> > 
> > 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")
> > 
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > ---
> > bug is specific to 21.11, no need to cc stable@dpdk.org
> > ---
> >  app/test/meson.build                     |   3 +
> >  app/test/test_vdev.c                     | 168 +++++++++++++++++++++++
> >  drivers/bus/auxiliary/auxiliary_params.c |   9 +-
> >  drivers/bus/vdev/vdev_params.c           |   9 +-
> >  4 files changed, 187 insertions(+), 2 deletions(-)
> >  create mode 100644 app/test/test_vdev.c
> > 
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index 961bebc5cb..19009da52e 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -153,6 +153,7 @@ test_sources = files(
> >          'test_trace.c',
> >          'test_trace_register.c',
> >          'test_trace_perf.c',
> > +        'test_vdev.c',
> >          'test_version.c',
> >          'virtual_pmd.c',
> >  )
> > @@ -177,6 +178,7 @@ test_deps = [
> >          'ipsec',
> >          'lpm',
> >          'member',
> > +        'net_null',
> 
> From my experience, CI will fail if build not configured with net_null,
> even we add it here. Let's monitor CI result. 

Sorry, from CI scripts, net_null was there already, no concern.

> 
> >          'node',
> >          'pipeline',
> >          'port',
> > @@ -283,6 +285,7 @@ fast_tests = [
> >          ['service_autotest', true],
> >          ['thash_autotest', true],
> >          ['trace_autotest', true],
> > +        ['vdev_autotest', true],
> >  ]
> >  
> >  # Tests known to have issues or which don't belong in other tests lists.
> > diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
> > new file mode 100644
> > index 0000000000..720722c363
> > --- /dev/null
> > +++ b/app/test/test_vdev.c
> > @@ -0,0 +1,168 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2021 6WIND S.A.
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +
> > +#include <rte_common.h>
> > +#include <rte_kvargs.h>
> > +#include <rte_bus_vdev.h>
> > +
> > +#include "test.h"
> > +
> > +#define TEST_VDEV_KEY_NAME "name"
> > +
> > +static const char * const valid_keys[] = {
> > +	TEST_VDEV_KEY_NAME,
> > +	NULL,
> > +};
> > +
> > +static int
> > +cmp_dev_name(const struct rte_device *dev, const void *name)
> > +{
> > +	return strcmp(dev->name, name);
> > +}
> > +
> > +static int
> > +cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
> > +{
> > +	const struct rte_kvargs *kvlist = _kvlist;
> > +	const char *key = TEST_VDEV_KEY_NAME;
> > +	const char *name;
> > +
> > +	/* no kvlist arg, all devices match */
> > +	if (kvlist == NULL)
> > +		return 0;
> > +
> > +	/* if key is present in kvlist and does not match, filter device */
> > +	name = rte_kvargs_get(kvlist, key);
> > +	if (name != NULL && strcmp(name, dev->name))
> > +		return -1;
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rte_device *
> > +get_matching_vdev(const char *match_str)
> > +{
> > +	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
> > +	struct rte_kvargs *kvargs = NULL;
> > +	struct rte_device *dev;
> > +
> > +	if (match_str != NULL) {
> > +		kvargs = rte_kvargs_parse(match_str, valid_keys);
> > +		if (kvargs == NULL) {
> > +			printf("Failed to parse match string\n");
> > +			return NULL;
> > +		}
> > +	}
> > +
> > +	dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs);
> > +	rte_kvargs_free(kvargs);
> > +
> > +	return dev;
> > +}
> > +
> > +static int
> > +test_vdev_bus(void)
> > +{
> > +	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
> > +	struct rte_dev_iterator dev_iter = { 0 };
> > +	struct rte_device *dev, *dev0, *dev1;
> > +
> > +	/* not supported */
> > +	if (vdev_bus == NULL)
> > +		return 0;
> > +
> > +	/* create first vdev */
> > +	if (rte_vdev_init("net_null_test0", "") < 0) {
> > +		printf("Failed to create vdev net_null_test0\n");
> > +		goto fail;
> > +	}
> > +	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
> > +	if (dev0 == NULL) {
> > +		printf("Cannot find net_null_test0 vdev\n");
> > +		goto fail;
> > +	}
> > +
> > +	/* create second vdev */
> > +	if (rte_vdev_init("net_null_test1", "") < 0) {
> > +		printf("Failed to create vdev net_null_test1\n");
> > +		goto fail;
> > +	}
> > +	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
> > +	if (dev1 == NULL) {
> > +		printf("Cannot find net_null_test1 vdev\n");
> > +		goto fail;
> > +	}
> > +
> > +	/* try to match vdevs */
> > +	dev = get_matching_vdev("name=net_null_test0");
> > +	if (dev != dev0) {
> > +		printf("Cannot match net_null_test0 vdev\n");
> > +		goto fail;
> > +	}
> > +
> > +	dev = get_matching_vdev("name=net_null_test1");
> > +	if (dev != dev1) {
> > +		printf("Cannot match net_null_test1 vdev\n");
> > +		goto fail;
> > +	}
> > +
> > +	dev = get_matching_vdev("name=unexistant");
> > +	if (dev != NULL) {
> > +		printf("Unexistant vdev should not match\n");
> > +		goto fail;
> > +	}
> > +
> > +	dev = get_matching_vdev("");
> > +	if (dev == NULL || dev == dev1) {
> > +		printf("Cannot match any vdev with empty match string\n");
> > +		goto fail;
> > +	}
> > +
> > +	dev = get_matching_vdev(NULL);
> > +	if (dev == NULL || dev == dev1) {
> > +		printf("Cannot match any vdev with NULL match string\n");
> > +		goto fail;
> > +	}
> > +
> > +	/* iterate all vdevs, and ensure we find vdev0 and vdev1 */
> > +	RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) {
> > +		if (dev == dev0)
> > +			dev0 = NULL;
> > +		else if (dev == dev1)
> > +			dev1 = NULL;
> > +	}
> > +	if (dev0 != NULL) {
> > +		printf("dev0 was not iterated\n");
> > +		goto fail;
> > +	}
> > +	if (dev1 != NULL) {
> > +		printf("dev1 was not iterated\n");
> > +		goto fail;
> > +	}
> > +
> > +	rte_vdev_uninit("net_null_test0");
> > +	rte_vdev_uninit("net_null_test1");
> > +
> > +	return 0;
> > +
> > +fail:
> > +	rte_vdev_uninit("net_null_test0");
> > +	rte_vdev_uninit("net_null_test1");
> > +	return -1;
> > +}
> > +
> > +static int
> > +test_vdev(void)
> > +{
> > +	printf("== test vdev bus ==\n");
> > +	if (test_vdev_bus() < 0)
> > +		return -1;
> > +	return 0;
> > +}
> > +
> > +REGISTER_TEST_COMMAND(vdev_autotest, test_vdev);
> > diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> > index 8dd8813611..9c08ccdd1b 100644
> > --- a/drivers/bus/auxiliary/auxiliary_params.c
> > +++ b/drivers/bus/auxiliary/auxiliary_params.c
> > @@ -28,8 +28,15 @@ auxiliary_dev_match(const struct rte_device *dev,
> >  {
> >  	const struct rte_kvargs *kvlist = _kvlist;
> >  	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
> > +	const char *name;
> >  
> > -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
> > +	/* no kvlist arg, all devices match */
> > +	if (kvlist == NULL)
> > +		return 0;
> > +
> > +	/* if key is present in kvlist and does not match, filter device */
> > +	name = rte_kvargs_get(kvlist, key);
> > +	if (name != NULL && strcmp(name, dev->name))
> >  		return -1;
> >  
> >  	return 0;
> > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> > index 37d95395e7..3969faf16d 100644
> > --- a/drivers/bus/vdev/vdev_params.c
> > +++ b/drivers/bus/vdev/vdev_params.c
> > @@ -28,8 +28,15 @@ vdev_dev_match(const struct rte_device *dev,
> >  {
> >  	const struct rte_kvargs *kvlist = _kvlist;
> >  	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
> > +	const char *name;
> >  
> > -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
> > +	/* no kvlist arg, all devices match */
> > +	if (kvlist == NULL)
> > +		return 0;
> > +
> > +	/* if key is present in kvlist and does not match, filter device */
> > +	name = rte_kvargs_get(kvlist, key);
> > +	if (name != NULL && strcmp(name, dev->name))
> >  		return -1;
> >  
> >  	return 0;
> 
> Nice unit test!
> 
> Reviewed-by: Xueming Li <xuemingl@nvidia.com>
> 

Re: [PATCH v3] bus: fix device iterator match from arguments

[Olivier Matz]

On Wed, Nov 24, 2021 at 11:43:29AM +0000, Xueming(Steven) Li wrote:
> On Wed, 2021-11-24 at 19:30 +0800, Xueming Li wrote:
> > On Wed, 2021-11-24 at 12:02 +0100, Olivier Matz wrote:
> > > From: Xueming Li <xuemingl@nvidia.com>
> > > 
> > > 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")
> > > 
> > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > > ---
> > > bug is specific to 21.11, no need to cc stable@dpdk.org
> > > ---
> > >  app/test/meson.build                     |   3 +
> > >  app/test/test_vdev.c                     | 168 +++++++++++++++++++++++
> > >  drivers/bus/auxiliary/auxiliary_params.c |   9 +-
> > >  drivers/bus/vdev/vdev_params.c           |   9 +-
> > >  4 files changed, 187 insertions(+), 2 deletions(-)
> > >  create mode 100644 app/test/test_vdev.c
> > > 
> > > diff --git a/app/test/meson.build b/app/test/meson.build
> > > index 961bebc5cb..19009da52e 100644
> > > --- a/app/test/meson.build
> > > +++ b/app/test/meson.build
> > > @@ -153,6 +153,7 @@ test_sources = files(
> > >          'test_trace.c',
> > >          'test_trace_register.c',
> > >          'test_trace_perf.c',
> > > +        'test_vdev.c',
> > >          'test_version.c',
> > >          'virtual_pmd.c',
> > >  )
> > > @@ -177,6 +178,7 @@ test_deps = [
> > >          'ipsec',
> > >          'lpm',
> > >          'member',
> > > +        'net_null',
> > 
> > From my experience, CI will fail if build not configured with net_null,
> > even we add it here. Let's monitor CI result. 
> 
> Sorry, from CI scripts, net_null was there already, no concern.

No you are right! I had almost the same comment from David offline.
I'm fixing the meson.build to add the test only when RTE_NET_NULL is set.

Thanks

> > 
> > >          'node',
> > >          'pipeline',
> > >          'port',
> > > @@ -283,6 +285,7 @@ fast_tests = [
> > >          ['service_autotest', true],
> > >          ['thash_autotest', true],
> > >          ['trace_autotest', true],
> > > +        ['vdev_autotest', true],
> > >  ]
> > >  
> > >  # Tests known to have issues or which don't belong in other tests lists.
> > > diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
> > > new file mode 100644
> > > index 0000000000..720722c363
> > > --- /dev/null
> > > +++ b/app/test/test_vdev.c
> > > @@ -0,0 +1,168 @@
> > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > + * Copyright 2021 6WIND S.A.
> > > + */
> > > +
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +
> > > +#include <rte_common.h>
> > > +#include <rte_kvargs.h>
> > > +#include <rte_bus_vdev.h>
> > > +
> > > +#include "test.h"
> > > +
> > > +#define TEST_VDEV_KEY_NAME "name"
> > > +
> > > +static const char * const valid_keys[] = {
> > > +	TEST_VDEV_KEY_NAME,
> > > +	NULL,
> > > +};
> > > +
> > > +static int
> > > +cmp_dev_name(const struct rte_device *dev, const void *name)
> > > +{
> > > +	return strcmp(dev->name, name);
> > > +}
> > > +
> > > +static int
> > > +cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
> > > +{
> > > +	const struct rte_kvargs *kvlist = _kvlist;
> > > +	const char *key = TEST_VDEV_KEY_NAME;
> > > +	const char *name;
> > > +
> > > +	/* no kvlist arg, all devices match */
> > > +	if (kvlist == NULL)
> > > +		return 0;
> > > +
> > > +	/* if key is present in kvlist and does not match, filter device */
> > > +	name = rte_kvargs_get(kvlist, key);
> > > +	if (name != NULL && strcmp(name, dev->name))
> > > +		return -1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct rte_device *
> > > +get_matching_vdev(const char *match_str)
> > > +{
> > > +	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
> > > +	struct rte_kvargs *kvargs = NULL;
> > > +	struct rte_device *dev;
> > > +
> > > +	if (match_str != NULL) {
> > > +		kvargs = rte_kvargs_parse(match_str, valid_keys);
> > > +		if (kvargs == NULL) {
> > > +			printf("Failed to parse match string\n");
> > > +			return NULL;
> > > +		}
> > > +	}
> > > +
> > > +	dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs);
> > > +	rte_kvargs_free(kvargs);
> > > +
> > > +	return dev;
> > > +}
> > > +
> > > +static int
> > > +test_vdev_bus(void)
> > > +{
> > > +	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
> > > +	struct rte_dev_iterator dev_iter = { 0 };
> > > +	struct rte_device *dev, *dev0, *dev1;
> > > +
> > > +	/* not supported */
> > > +	if (vdev_bus == NULL)
> > > +		return 0;
> > > +
> > > +	/* create first vdev */
> > > +	if (rte_vdev_init("net_null_test0", "") < 0) {
> > > +		printf("Failed to create vdev net_null_test0\n");
> > > +		goto fail;
> > > +	}
> > > +	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
> > > +	if (dev0 == NULL) {
> > > +		printf("Cannot find net_null_test0 vdev\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	/* create second vdev */
> > > +	if (rte_vdev_init("net_null_test1", "") < 0) {
> > > +		printf("Failed to create vdev net_null_test1\n");
> > > +		goto fail;
> > > +	}
> > > +	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
> > > +	if (dev1 == NULL) {
> > > +		printf("Cannot find net_null_test1 vdev\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	/* try to match vdevs */
> > > +	dev = get_matching_vdev("name=net_null_test0");
> > > +	if (dev != dev0) {
> > > +		printf("Cannot match net_null_test0 vdev\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	dev = get_matching_vdev("name=net_null_test1");
> > > +	if (dev != dev1) {
> > > +		printf("Cannot match net_null_test1 vdev\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	dev = get_matching_vdev("name=unexistant");
> > > +	if (dev != NULL) {
> > > +		printf("Unexistant vdev should not match\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	dev = get_matching_vdev("");
> > > +	if (dev == NULL || dev == dev1) {
> > > +		printf("Cannot match any vdev with empty match string\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	dev = get_matching_vdev(NULL);
> > > +	if (dev == NULL || dev == dev1) {
> > > +		printf("Cannot match any vdev with NULL match string\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	/* iterate all vdevs, and ensure we find vdev0 and vdev1 */
> > > +	RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) {
> > > +		if (dev == dev0)
> > > +			dev0 = NULL;
> > > +		else if (dev == dev1)
> > > +			dev1 = NULL;
> > > +	}
> > > +	if (dev0 != NULL) {
> > > +		printf("dev0 was not iterated\n");
> > > +		goto fail;
> > > +	}
> > > +	if (dev1 != NULL) {
> > > +		printf("dev1 was not iterated\n");
> > > +		goto fail;
> > > +	}
> > > +
> > > +	rte_vdev_uninit("net_null_test0");
> > > +	rte_vdev_uninit("net_null_test1");
> > > +
> > > +	return 0;
> > > +
> > > +fail:
> > > +	rte_vdev_uninit("net_null_test0");
> > > +	rte_vdev_uninit("net_null_test1");
> > > +	return -1;
> > > +}
> > > +
> > > +static int
> > > +test_vdev(void)
> > > +{
> > > +	printf("== test vdev bus ==\n");
> > > +	if (test_vdev_bus() < 0)
> > > +		return -1;
> > > +	return 0;
> > > +}
> > > +
> > > +REGISTER_TEST_COMMAND(vdev_autotest, test_vdev);
> > > diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
> > > index 8dd8813611..9c08ccdd1b 100644
> > > --- a/drivers/bus/auxiliary/auxiliary_params.c
> > > +++ b/drivers/bus/auxiliary/auxiliary_params.c
> > > @@ -28,8 +28,15 @@ auxiliary_dev_match(const struct rte_device *dev,
> > >  {
> > >  	const struct rte_kvargs *kvlist = _kvlist;
> > >  	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
> > > +	const char *name;
> > >  
> > > -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
> > > +	/* no kvlist arg, all devices match */
> > > +	if (kvlist == NULL)
> > > +		return 0;
> > > +
> > > +	/* if key is present in kvlist and does not match, filter device */
> > > +	name = rte_kvargs_get(kvlist, key);
> > > +	if (name != NULL && strcmp(name, dev->name))
> > >  		return -1;
> > >  
> > >  	return 0;
> > > diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> > > index 37d95395e7..3969faf16d 100644
> > > --- a/drivers/bus/vdev/vdev_params.c
> > > +++ b/drivers/bus/vdev/vdev_params.c
> > > @@ -28,8 +28,15 @@ vdev_dev_match(const struct rte_device *dev,
> > >  {
> > >  	const struct rte_kvargs *kvlist = _kvlist;
> > >  	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
> > > +	const char *name;
> > >  
> > > -	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
> > > +	/* no kvlist arg, all devices match */
> > > +	if (kvlist == NULL)
> > > +		return 0;
> > > +
> > > +	/* if key is present in kvlist and does not match, filter device */
> > > +	name = rte_kvargs_get(kvlist, key);
> > > +	if (name != NULL && strcmp(name, dev->name))
> > >  		return -1;
> > >  
> > >  	return 0;
> > 
> > Nice unit test!
> > 
> > Reviewed-by: Xueming Li <xuemingl@nvidia.com>
> > 
> 

[PATCH v4] bus: fix device iterator match from arguments

[Olivier Matz]

From: Xueming Li <xuemingl@nvidia.com>

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

Signed-off-by: Xueming Li <xuemingl@nvidia.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Reviewed-by: Xueming Li <xuemingl@nvidia.com>
---
bug is specific to 21.11, no need to cc stable@dpdk.org

v4:
- disable unit test when net/null is not enabled
v3:
- add unit test
v2:
- use rte_kvargs_get() + strcmp instead of rte_kvargs_get_with_value()
---
 app/test/meson.build                     |   5 +
 app/test/test_vdev.c                     | 168 +++++++++++++++++++++++
 drivers/bus/auxiliary/auxiliary_params.c |   9 +-
 drivers/bus/vdev/vdev_params.c           |   9 +-
 4 files changed, 189 insertions(+), 2 deletions(-)
 create mode 100644 app/test/test_vdev.c

diff --git a/app/test/meson.build b/app/test/meson.build
index 961bebc5cb..2b480adfba 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -426,6 +426,11 @@ if dpdk_conf.has('RTE_NET_RING')
         fast_tests += [['pdump_autotest', true]]
     endif
 endif
+if dpdk_conf.has('RTE_NET_NULL')
+    test_deps += 'net_null'
+    test_sources += 'test_vdev.c'
+    fast_tests += [['vdev_autotest', true]]
+endif
 
 if dpdk_conf.has('RTE_HAS_LIBPCAP')
     ext_deps += pcap_dep
diff --git a/app/test/test_vdev.c b/app/test/test_vdev.c
new file mode 100644
index 0000000000..720722c363
--- /dev/null
+++ b/app/test/test_vdev.c
@@ -0,0 +1,168 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 6WIND S.A.
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <rte_common.h>
+#include <rte_kvargs.h>
+#include <rte_bus_vdev.h>
+
+#include "test.h"
+
+#define TEST_VDEV_KEY_NAME "name"
+
+static const char * const valid_keys[] = {
+	TEST_VDEV_KEY_NAME,
+	NULL,
+};
+
+static int
+cmp_dev_name(const struct rte_device *dev, const void *name)
+{
+	return strcmp(dev->name, name);
+}
+
+static int
+cmp_dev_match(const struct rte_device *dev, const void *_kvlist)
+{
+	const struct rte_kvargs *kvlist = _kvlist;
+	const char *key = TEST_VDEV_KEY_NAME;
+	const char *name;
+
+	/* no kvlist arg, all devices match */
+	if (kvlist == NULL)
+		return 0;
+
+	/* if key is present in kvlist and does not match, filter device */
+	name = rte_kvargs_get(kvlist, key);
+	if (name != NULL && strcmp(name, dev->name))
+		return -1;
+
+	return 0;
+}
+
+static struct rte_device *
+get_matching_vdev(const char *match_str)
+{
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+	struct rte_kvargs *kvargs = NULL;
+	struct rte_device *dev;
+
+	if (match_str != NULL) {
+		kvargs = rte_kvargs_parse(match_str, valid_keys);
+		if (kvargs == NULL) {
+			printf("Failed to parse match string\n");
+			return NULL;
+		}
+	}
+
+	dev = vdev_bus->find_device(NULL, cmp_dev_match, kvargs);
+	rte_kvargs_free(kvargs);
+
+	return dev;
+}
+
+static int
+test_vdev_bus(void)
+{
+	struct rte_bus *vdev_bus = rte_bus_find_by_name("vdev");
+	struct rte_dev_iterator dev_iter = { 0 };
+	struct rte_device *dev, *dev0, *dev1;
+
+	/* not supported */
+	if (vdev_bus == NULL)
+		return 0;
+
+	/* create first vdev */
+	if (rte_vdev_init("net_null_test0", "") < 0) {
+		printf("Failed to create vdev net_null_test0\n");
+		goto fail;
+	}
+	dev0 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test0");
+	if (dev0 == NULL) {
+		printf("Cannot find net_null_test0 vdev\n");
+		goto fail;
+	}
+
+	/* create second vdev */
+	if (rte_vdev_init("net_null_test1", "") < 0) {
+		printf("Failed to create vdev net_null_test1\n");
+		goto fail;
+	}
+	dev1 = vdev_bus->find_device(NULL, cmp_dev_name, "net_null_test1");
+	if (dev1 == NULL) {
+		printf("Cannot find net_null_test1 vdev\n");
+		goto fail;
+	}
+
+	/* try to match vdevs */
+	dev = get_matching_vdev("name=net_null_test0");
+	if (dev != dev0) {
+		printf("Cannot match net_null_test0 vdev\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev("name=net_null_test1");
+	if (dev != dev1) {
+		printf("Cannot match net_null_test1 vdev\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev("name=unexistant");
+	if (dev != NULL) {
+		printf("Unexistant vdev should not match\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev("");
+	if (dev == NULL || dev == dev1) {
+		printf("Cannot match any vdev with empty match string\n");
+		goto fail;
+	}
+
+	dev = get_matching_vdev(NULL);
+	if (dev == NULL || dev == dev1) {
+		printf("Cannot match any vdev with NULL match string\n");
+		goto fail;
+	}
+
+	/* iterate all vdevs, and ensure we find vdev0 and vdev1 */
+	RTE_DEV_FOREACH(dev, "bus=vdev", &dev_iter) {
+		if (dev == dev0)
+			dev0 = NULL;
+		else if (dev == dev1)
+			dev1 = NULL;
+	}
+	if (dev0 != NULL) {
+		printf("dev0 was not iterated\n");
+		goto fail;
+	}
+	if (dev1 != NULL) {
+		printf("dev1 was not iterated\n");
+		goto fail;
+	}
+
+	rte_vdev_uninit("net_null_test0");
+	rte_vdev_uninit("net_null_test1");
+
+	return 0;
+
+fail:
+	rte_vdev_uninit("net_null_test0");
+	rte_vdev_uninit("net_null_test1");
+	return -1;
+}
+
+static int
+test_vdev(void)
+{
+	printf("== test vdev bus ==\n");
+	if (test_vdev_bus() < 0)
+		return -1;
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(vdev_autotest, test_vdev);
diff --git a/drivers/bus/auxiliary/auxiliary_params.c b/drivers/bus/auxiliary/auxiliary_params.c
index 8dd8813611..9c08ccdd1b 100644
--- a/drivers/bus/auxiliary/auxiliary_params.c
+++ b/drivers/bus/auxiliary/auxiliary_params.c
@@ -28,8 +28,15 @@ auxiliary_dev_match(const struct rte_device *dev,
 {
 	const struct rte_kvargs *kvlist = _kvlist;
 	const char *key = auxiliary_params_keys[RTE_AUXILIARY_PARAM_NAME];
+	const char *name;
 
-	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
+	/* no kvlist arg, all devices match */
+	if (kvlist == NULL)
+		return 0;
+
+	/* if key is present in kvlist and does not match, filter device */
+	name = rte_kvargs_get(kvlist, key);
+	if (name != NULL && strcmp(name, dev->name))
 		return -1;
 
 	return 0;
diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
index 37d95395e7..3969faf16d 100644
--- a/drivers/bus/vdev/vdev_params.c
+++ b/drivers/bus/vdev/vdev_params.c
@@ -28,8 +28,15 @@ vdev_dev_match(const struct rte_device *dev,
 {
 	const struct rte_kvargs *kvlist = _kvlist;
 	const char *key = vdev_params_keys[RTE_VDEV_PARAM_NAME];
+	const char *name;
 
-	if (rte_kvargs_get_with_value(kvlist, key, dev->name) == NULL)
+	/* no kvlist arg, all devices match */
+	if (kvlist == NULL)
+		return 0;
+
+	/* if key is present in kvlist and does not match, filter device */
+	name = rte_kvargs_get(kvlist, key);
+	if (name != NULL && strcmp(name, dev->name))
 		return -1;
 
 	return 0;
-- 
2.30.2

Re: [PATCH v4] bus: fix device iterator match from arguments

[Xueming(Steven) Li]

On Wed, 2021-11-24 at 13:45 +0100, Olivier Matz wrote:
> From: Xueming Li <xuemingl@nvidia.com>
> 
> 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")
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Reviewed-by: Xueming Li <xuemingl@nvidia.com>
> ---
> bug is specific to 21.11, no need to cc stable@dpdk.org
> 
> v4:
> - disable unit test when net/null is not enabled
> v3:
> - add unit test
> v2:
> - use rte_kvargs_get() + strcmp instead of rte_kvargs_get_with_value()

Acked-by: Xueming Li <xuemingl@nvidia.com>

Re: [PATCH v4] bus: fix device iterator match from arguments

[David Marchand]

On Wed, Nov 24, 2021 at 2:00 PM Xueming(Steven) Li <xuemingl@nvidia.com> wrote:
> On Wed, 2021-11-24 at 13:45 +0100, Olivier Matz wrote:
> > From: Xueming Li <xuemingl@nvidia.com>
> >
> > 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")
> >
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Acked-by: Xueming Li <xuemingl@nvidia.com>

Added test_vdev.c in MAINTAINERS vdev bus section.

Applied, thanks.


-- 
David Marchand