On Fri, Jul 14, 2017 at 05:12:11PM -0400, Jan Blunck wrote:
> Let the rte_eal_devargs_parse() function explicitly take a "busname"
> argument that is validated.
>
> Now that the busname is known and validated at parse time the validity of
> the device name is checked for all device types when they get probed.
>
What use is there for a parsing API if the fields have already been
recognized? Why would someone need to parse an rte_devargs if they already
know the busname from the devname? This function becomes useless.
You are applying here the same logic you used with the rte_dev API,
where you divided the fields because it made no sense to have them
merged.
However, the rte_dev API is not in contact with users / applications, it
is not the interface with the outside world where it must take in external
inputs and format them for subsequent systems to use.
> Signed-off-by: Jan Blunck <jblu...@infradead.org>
> ---
> lib/librte_eal/common/eal_common_devargs.c | 72
> +++++++++++------------------
> lib/librte_eal/common/include/rte_devargs.h | 17 ++++---
> test/test/test_devargs.c | 21 +++------
> 3 files changed, 45 insertions(+), 65 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_devargs.c
> b/lib/librte_eal/common/eal_common_devargs.c
> index 38ba64567..44a8d8562 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -77,30 +77,21 @@ rte_eal_parse_devargs_str(const char *devargs_str,
> return 0;
> }
>
> -static int
> -bus_name_cmp(const struct rte_bus *bus, const void *name)
> -{
> - return strncmp(bus->name, name, strlen(bus->name));
> -}
> -
> int
> -rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
> +rte_eal_devargs_parse(const char *busname, const char *dev,
> + struct rte_devargs *da)
> {
> - struct rte_bus *bus = NULL;
> char *devname = NULL, *drvargs = NULL;
> int ret;
>
> - if (dev == NULL || da == NULL)
> + if (busname == NULL || dev == NULL || da == NULL)
> return -EINVAL;
> /* Retrieve eventual bus info */
> - do {
> - bus = rte_bus_find(bus, bus_name_cmp, dev);
> - if (bus == NULL)
> - break;
> - dev += strlen(bus->name) + 1;
> - if (rte_bus_find_by_device_name(dev) == bus)
> - break;
> - } while (1);
> + da->bus = rte_bus_find_by_name(busname);
> + if (da->bus == NULL) {
> + RTE_LOG(ERR, EAL, "Bus not found: \"%s\"\n", busname);
> + return -EFAULT;
> + }
>
> ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
> if (ret != 0)
> @@ -118,21 +109,10 @@ rte_eal_devargs_parse(const char *dev, struct
> rte_devargs *da)
> da->args = drvargs;
> drvargs = NULL;
>
> - if (bus == NULL) {
> - bus = rte_bus_find_by_device_name(da->name);
> - if (bus == NULL) {
> - RTE_LOG(ERR, EAL, "Failed to parse device: \"%s\"\n",
> - da->name);
> - ret = -EFAULT;
> - goto fail;
> - }
> - }
> - da->bus = bus;
> -
> /* Store bus name */
> - ret = snprintf(da->busname, sizeof(da->busname), "%s", bus->name);
> + ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
> if (ret < 0 || ret >= (int)sizeof(da->busname)) {
> - RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", bus->name);
> + RTE_LOG(ERR, EAL, "Invalid bus name: \"%s\"\n", busname);
> ret = -EINVAL;
> goto fail;
> }
> @@ -158,8 +138,7 @@ int
> rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> {
> struct rte_devargs *devargs = NULL;
> - struct rte_bus *bus = NULL;
> - const char *dev = devargs_str;
> + const char *busname = NULL;
> int ret;
>
> /* use calloc instead of rte_zmalloc as it's called early at init */
> @@ -167,31 +146,36 @@ rte_eal_devargs_add(enum rte_devtype devtype, const
> char *devargs_str)
> if (devargs == NULL)
> goto fail;
>
> - if (rte_eal_devargs_parse(dev, devargs))
> - goto fail;
> - devargs->type = devtype;
> - bus = devargs->bus;
> - if (devtype != RTE_DEVTYPE_VIRTUAL) {
> - ret = rte_bus_configure(bus,
> + switch (devtype) {
> + case RTE_DEVTYPE_WHITELISTED_PCI:
> + case RTE_DEVTYPE_BLACKLISTED_PCI:
> + busname = "pci";
> + ret = rte_bus_configure(rte_bus_find_by_name(busname),
> devtype == RTE_DEVTYPE_WHITELISTED_PCI ?
> &BUS_CONF_WHITELIST : &BUS_CONF_BLACKLIST);
> if (ret != 0) {
> RTE_LOG(ERR, EAL,
> "Bus [%s] scan_mode conflicts with devtype: "
> - "%s\n", bus->name, devargs_str);
> + "%s\n", busname, devargs_str);
> goto fail;
> }
> + break;
> + case RTE_DEVTYPE_VIRTUAL:
> + busname = "vdev";
> + break;
> }
>
> + if (rte_eal_devargs_parse(busname, devargs_str, devargs))
> + goto fail;
> +
> + RTE_LOG(DEBUG, EAL, "Adding devargs for device [%s] on bus [%s]: %s\n",
> + devargs->name, busname, devargs->args);
> +
> TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
> return 0;
>
> fail:
> - if (devargs) {
> - free(devargs->args);
> - free(devargs);
> - }
> -
> + free(devargs);
> return -1;
> }
>
> diff --git a/lib/librte_eal/common/include/rte_devargs.h
> b/lib/librte_eal/common/include/rte_devargs.h
> index 8dafb167e..10953327f 100644
> --- a/lib/librte_eal/common/include/rte_devargs.h
> +++ b/lib/librte_eal/common/include/rte_devargs.h
> @@ -124,10 +124,12 @@ int rte_eal_parse_devargs_str(const char *devargs_str,
> /**
> * Parse a device string.
> *
> - * Verify that a bus is capable of handling the device passed
> - * in argument. Store which bus will handle the device, its name
> - * and the eventual device parameters.
> + * The function parses the arguments string to get driver name and driver
> + * arguments and stores together with the busname that will handle the
> device,
> + * its name and the eventual device parameters.
> *
> + * @param busname
> + * The bus name the device declaration string applies to.
> * @param dev
> * The device declaration string.
> * @param da
> @@ -138,7 +140,7 @@ int rte_eal_parse_devargs_str(const char *devargs_str,
> * - Negative errno on error.
> */
> int
> -rte_eal_devargs_parse(const char *dev,
> +rte_eal_devargs_parse(const char *busname, const char *dev,
> struct rte_devargs *da);
>
> /**
> @@ -151,9 +153,10 @@ rte_eal_devargs_parse(const char *dev,
> *
> * For virtual devices, the format of arguments string is "DRIVER_NAME*"
> * or "DRIVER_NAME*,key=val,key2=val2,...". Examples: "net_ring",
> - * "net_ring0", "net_pmdAnything,arg=0:arg2=1". The validity of the
> - * driver name is not checked by this function, it is done when probing
> - * the drivers.
> + * "net_ring0", "net_pmdAnything,arg=0:arg2=1".
> + *
> + * The validity of the device name is not checked by this function, it is
> done
> + * when probing the drivers.
> *
> * @param devtype
> * The type of the device.
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 9e0ff5995..84803cf5a 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -110,24 +110,17 @@ test_devargs(void)
> goto fail;
> free_devargs_list();
>
> - /* test error case: bad PCI address */
> - if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "08:1") == 0)
> - goto fail;
> - if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "00.1") == 0)
> - goto fail;
> - if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "foo") == 0)
> - goto fail;
> - if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, ",") == 0)
> - goto fail;
> - if (rte_eal_devargs_add(RTE_DEVTYPE_WHITELISTED_PCI, "000f:0:0") == 0)
> - goto fail;
> -
> - if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
> + if (rte_eal_devargs_parse("", "", &devargs_stack) == 0) {
> printf("Error in rte_eal_devargs_parse()\n");
> goto fail;
> }
>
> - if (rte_eal_devargs_parse("08:00.1,foo=bar", &devargs_stack) != 0) {
> + if (rte_eal_devargs_parse("vdev", "not_found", &devargs_stack) != 0) {
> + printf("Error in rte_eal_devargs_parse(vdev:not_found)\n");
> + goto fail;
> + }
> + if (rte_eal_devargs_parse("pci", "08:00.1,foo=bar",
> + &devargs_stack) != 0) {
> printf("Error in rte_eal_devargs_parse(08:00.1,foo=bar)\n");
> goto fail;
> }
> --
> 2.13.2
>
--
Gaëtan Rivet
6WIND
