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 <tho...@monjalon.net>
> Reviewed-by: Andrew Rybchenko <arybche...@solarflare.com>
With the following remark being taken care of,
Acked-by: Gaetan Rivet <gaetan.ri...@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
