Re: [dpdk-dev] [PATCH 1/5] bus/vdev: add iteration filter on name

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/8/18 1:25 AM, Thomas Monjalon wrote:
> A virtual device can be matched with following syntax:
> 	bus=vdev,name=X
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   drivers/bus/vdev/vdev_params.c | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bus/vdev/vdev_params.c b/drivers/bus/vdev/vdev_params.c
> index da270f2ec..133998c3e 100644
> --- a/drivers/bus/vdev/vdev_params.c
> +++ b/drivers/bus/vdev/vdev_params.c
> @@ -2,6 +2,8 @@
>    * Copyright 2018 Gaëtan Rivet
>    */
>   
> +#include <string.h>
> +
>   #include <rte_dev.h>
>   #include <rte_bus.h>
>   #include <rte_kvargs.h>
> @@ -11,10 +13,12 @@
>   #include "vdev_private.h"
>   
>   enum vdev_params {
> +	RTE_VDEV_PARAM_NAME,
>   	RTE_VDEV_PARAM_MAX,
>   };
>   
>   static const char * const vdev_params_keys[] = {
> +	[RTE_VDEV_PARAM_NAME] = "name",
>   	[RTE_VDEV_PARAM_MAX] = NULL,
>   };
>   
> @@ -22,11 +26,22 @@ static int
>   vdev_dev_match(const struct rte_device *dev,
>   	       const void *_kvlist)
>   {
> +	int ret;
>   	const struct rte_kvargs *kvlist = _kvlist;
> +	char *name;
> +
> +	/* cannot pass const dev->name to rte_kvargs_process() */
> +	name = strdup(dev->name);
> +	if (name == NULL)
> +		return -ENOMEM; /* interpreted as no match */

It is strange to see -ENOMEM and -1 returned from the same function.
rte_dev_cmp_t does not return negative errno. It just says match /
no match (greater / smaller than 0 if ordering is possible).
So, -ENOMEM is really confusing here. I think just -1 should be used.

> +	ret = rte_kvargs_process(kvlist,
> +		vdev_params_keys[RTE_VDEV_PARAM_NAME],
> +		rte_kvargs_strcmp, name);
> +	free(name);
> +	if (ret != 0)
> +		return -1;
>   
> -	(void) kvlist;
> -	(void) dev;
> -	return 0;
> +	return ret;

I'm not sure that I understand why 'ret' is returned here
instead of 0. Above check guarantees that ret==0.
If you change it, it should be a good reason.

>   }
>   
>   void *