[dpdk-dev,v2,13/15] devargs: pass busname argument when parsing
Checks
Commit Message
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.
Signed-off-by: Jan Blunck <jblunck@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(-)
Comments
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 <jblunck@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
>
On 7/14/2017 10:12 PM, 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.
>
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
<...>
> --- 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;
Why removed these cases?
<...>
On 7/15/2017 3:48 PM, Gaëtan Rivet wrote:
> 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.
rte_eal_devargs_parse() converts devargs_string to the devargs struct,
this usage is still valid.
Getting busname as argument to this function looks good simplification
to me, I didn't get the problem here.
>
> 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 <jblunck@infradead.org>
<...>
@@ -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;
}
@@ -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.
@@ -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;
}