Re: [dpdk-dev] [PATCH v5 5/5] eal: simplify parameters of hotplug functions

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

I much prefer this API,

I have one remark inline however.

On Thu, Oct 04, 2018 at 01:10:46AM +0200, Thomas Monjalon wrote:
> All information about a device to probe can be grouped
> in a common string, which is what we usually call devargs.
> An application should not have to parse this string before
> calling the EAL probe function.
> And the syntax could evolve to be more complex and support
> matching multiple devices in one string.
> That's why the bus name and device name should be removed from
> rte_eal_hotplug_add().
> Instead of changing this function, a simpler one is added
> and used in the old one, which may be deprecated later.
> 
> When removing a device, we already know its rte_device handle
> which can be directly passed as parameter of rte_eal_hotplug_remove().
> If the rte_device is not known, it can be retrieved with the devargs,
> by iterating in the device list (future RTE_DEV_FOREACH()).
> Similarly to the probing case, a new function is added
> and used in the old one, which may be deprecated later.
> The new function is used in failsafe, because the replacement is easy.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

With the following remark being taken care of,

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

> ---
>  drivers/net/failsafe/failsafe_eal.c     |  3 +-
>  drivers/net/failsafe/failsafe_ether.c   |  3 +-
>  lib/librte_eal/common/eal_common_dev.c  | 80 ++++++++++++++++---------
>  lib/librte_eal/common/include/rte_dev.h | 30 +++++++++-
>  lib/librte_eal/rte_eal_version.map      |  2 +
>  5 files changed, 83 insertions(+), 35 deletions(-)
> 

<snip>

> diff --git a/lib/librte_eal/common/eal_common_dev.c b/lib/librte_eal/common/eal_common_dev.c
> index ce6d145fb..5dcc1403f 100644
> --- a/lib/librte_eal/common/eal_common_dev.c
> +++ b/lib/librte_eal/common/eal_common_dev.c
> @@ -129,46 +129,61 @@ int rte_eal_dev_detach(struct rte_device *dev)
>  
>  int
>  rte_eal_hotplug_add(const char *busname, const char *devname,
> -		    const char *devargs)
> +		    const char *drvargs)
>  {
> -	struct rte_bus *bus;
> -	struct rte_device *dev;
> -	struct rte_devargs *da;
>  	int ret;
> +	char *devargs = NULL;
> +	int size, length = -1;
>  
> -	bus = rte_bus_find_by_name(busname);
> -	if (bus == NULL) {
> -		RTE_LOG(ERR, EAL, "Cannot find bus (%s)\n", busname);
> -		return -ENOENT;
> -	}
> +	do { /* 2 iterations: first is to know string length */
> +		size = length + 1;
> +		length = snprintf(devargs, size, "%s:%s,%s", busname, devname, drvargs);
> +		if (length >= size)
> +			devargs = malloc(length + 1);
> +		if (devargs == NULL)
> +			return -ENOMEM;
> +	} while (size == 0);

I don't see a good reason for writing it this way.
You use an additional state (size) and complicate something that is
pretty straightforward. To make sure no error was written here, I had to
do step-by-step careful reading, this should not be required by clean
code.

You should remove this loop, which then allow removing size, and forces using
simple code-flow.

>  
> -	if (bus->plug == NULL) {
> -		RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
> -			bus->name);
> -		return -ENOTSUP;
> -	}
> +	ret = rte_dev_probe(devargs);
> +
> +	free(devargs);
> +	return ret;
> +}
> +
> +int __rte_experimental
> +rte_dev_probe(const char *devargs)
> +{
> +	struct rte_device *dev;
> +	struct rte_devargs *da;
> +	int ret;
>  
>  	da = calloc(1, sizeof(*da));
>  	if (da == NULL)
>  		return -ENOMEM;
>  
> -	ret = rte_devargs_parsef(da, "%s:%s,%s",
> -				 busname, devname, devargs);
> +	ret = rte_devargs_parse(da, devargs);
>  	if (ret)
>  		goto err_devarg;
>  
> +	if (da->bus->plug == NULL) {
> +		RTE_LOG(ERR, EAL, "Function plug not supported by bus (%s)\n",
> +			da->bus->name);
> +		ret = -ENOTSUP;
> +		goto err_devarg;
> +	}
> +
>  	ret = rte_devargs_insert(da);
>  	if (ret)
>  		goto err_devarg;
>  
> -	ret = bus->scan();
> +	ret = da->bus->scan();
>  	if (ret)
>  		goto err_devarg;
>  
> -	dev = bus->find_device(NULL, cmp_dev_name, devname);
> +	dev = da->bus->find_device(NULL, cmp_dev_name, da->name);
>  	if (dev == NULL) {
>  		RTE_LOG(ERR, EAL, "Cannot find device (%s)\n",
> -			devname);
> +			da->name);
>  		ret = -ENODEV;
>  		goto err_devarg;
>  	}

<snip>

-- 
Gaëtan Rivet
6WIND