[v5,5/5] eal: simplify parameters of hotplug functions

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

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>
  

04/10/2018 11:44, Gaëtan Rivet:
> On Thu, Oct 04, 2018 at 01:10:46AM +0200, Thomas Monjalon wrote:
> > --- 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.

The only reason for this loop is to avoid duplicating the snprintf format
in two calls.

It could be replaced by this:

	length = snprintf(devargs, 0, "%s:%s,%s", busname, devname, drvargs);
	if (length < 0)
		return -EINVAL;
	devargs = malloc(length + 1);
	if (devargs == NULL)
		return -ENOMEM;
	snprintf(devargs, length + 1, "%s:%s,%s", busname, devname, drvargs);

Any preference?
  

On Thu, Oct 04, 2018 at 01:46:54PM +0200, Thomas Monjalon wrote:
> 04/10/2018 11:44, Gaëtan Rivet:
> > On Thu, Oct 04, 2018 at 01:10:46AM +0200, Thomas Monjalon wrote:
> > > --- 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.
> 
> The only reason for this loop is to avoid duplicating the snprintf format
> in two calls.
> 
> It could be replaced by this:
> 
> 	length = snprintf(devargs, 0, "%s:%s,%s", busname, devname, drvargs);
> 	if (length < 0)
> 		return -EINVAL;
> 	devargs = malloc(length + 1);
> 	if (devargs == NULL)
> 		return -ENOMEM;
> 	snprintf(devargs, length + 1, "%s:%s,%s", busname, devname, drvargs);
> 
> Any preference?
> 
> 

Yes, the second is cleaner IMO.