[dpdk-dev,09/13] devargs: parse "bus=" argument
Checks
Commit Message
Let the rte_eal_devargs_parse() function parse the "bus=" argument.
Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
test/test/test_devargs.c | 19 +++++
2 files changed, 102 insertions(+), 48 deletions(-)
Comments
I still disagree with parsing the body of devargs to retrieve bus
information. They should not be mixed.
This conceptual discrepancy can be seen plainly in the code structure:
you emulate librte_kvargs because you do not want to depend on it within
the EAL, but copy its behavior and are forced to rewrite it partially.
Can you justify this? The commitlog does not describe the problem being
solved.
I think there should be a discussion about the future format of device
declarations. One version has been integrated in RC1 but it will be reworked
anyway. I'd like to hear more opinions.
On Tue, Jul 11, 2017 at 07:25:08PM -0400, Jan Blunck wrote:
> Let the rte_eal_devargs_parse() function parse the "bus=" argument.
>
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> ---
> lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
> test/test/test_devargs.c | 19 +++++
> 2 files changed, 102 insertions(+), 48 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 2bdee9a30..a0d0e6e91 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
> return 0;
> }
>
> +/*
> + * Parse "bus=" argument without adding a dependency on rte_kvargs.h
> + */
> +static char *parse_bus_arg(const char *args)
> +{
> + const char *c;
> + char *str = NULL;
> +
> + if (!args || args[0] == '\0')
> + return NULL;
> +
> + c = args;
> +
> + do {
> + if (strncmp(c, "bus=", 4) == 0) {
> + c += 4;
> + break;
> + }
> +
> + c = strchr(c, ',');
> + if (c)
> + c++;
> + } while (c);
> +
> + if (c) {
> + str = strdup(c);
> + c = strchr(str, ',');
> + if (c)
> + c = '\0';
> + }
> +
> + return str;
> +}
> +
> static int
> devargs_add(const char *busname, const char *name, const char *args)
> {
> @@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
> return -1;
> }
>
> -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)
> {
> - struct rte_bus *bus = NULL;
> - const char *devname;
> - const size_t maxlen = sizeof(da->name);
> - size_t i;
> + char *busname;
> + char *devname;
> + char *drvargs;
> + int ret;
>
> if (dev == NULL || da == NULL)
> return -EINVAL;
> - /* Retrieve eventual bus info */
> - do {
> - devname = dev;
> - bus = rte_bus_find(bus, bus_name_cmp, dev);
> - if (bus == NULL)
> - break;
> - devname = dev + strlen(bus->name) + 1;
> - if (rte_bus_find_by_device_name(devname) == bus)
> - break;
> - } while (1);
> - /* Store device name */
> - i = 0;
> - while (devname[i] != '\0' && devname[i] != ',') {
> - da->name[i] = devname[i];
> - i++;
> - if (i == maxlen) {
> - fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
> - dev, maxlen);
> - da->name[i - 1] = '\0';
> - return -EINVAL;
> - }
> - }
> - da->name[i] = '\0';
> - if (bus == NULL) {
> - bus = rte_bus_find_by_device_name(da->name);
> +
> + ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
> + if (ret != 0)
> + return ret;
> +
> + RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
> + __FUNCTION__, devname, drvargs);
> +
> + /* Retrieve eventual bus info (...,bus=...)*/
> + busname = parse_bus_arg(drvargs);
> + if (busname == NULL) {
> + struct rte_bus *bus;
> +
> + bus = rte_bus_find_by_device_name(devname);
> if (bus == NULL) {
> - fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
> - da->name);
> - return -EFAULT;
> + RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
> + devname);
> + ret = -EINVAL;
> + goto fail;
> }
> +
> + busname = strdup(bus->name);
> }
> - da->bus = bus;
> - /* Parse eventual device arguments */
> - if (devname[i] == ',')
> - da->args = strdup(&devname[i + 1]);
> - else
> - da->args = strdup("");
> - if (da->args == NULL) {
> - fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
> - return -ENOMEM;
> +
> + ret = devargs_add(busname, devname, drvargs);
> + if (ret != 0) {
> + RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
> + devname);
> + goto fail;
> }
> - return 0;
> +
> + ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
> + if (ret < 0 || ret >= (int)sizeof(da->busname))
> + goto fail;
> +
> + ret = snprintf(da->name, sizeof(da->name), "%s", devname);
> + if (ret < 0 || ret >= (int)sizeof(da->name))
> + goto fail;
> +
> + da->args = strdup(drvargs);
> + if (da->args == NULL)
> + ret = -ENOMEM;
> + ret = 0;
> +
> +fail:
> + free(busname);
> + free(devname);
> + free(drvargs);
> + return ret;
> }
>
> static const struct rte_bus_conf BUS_CONF_WHITELIST = {
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 048b3d00b..3a1033ca3 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -58,6 +58,7 @@ test_devargs(void)
> {
> struct rte_devargs_list save_devargs_list;
> struct rte_devargs *devargs;
> + struct rte_devargs devargs_stack;
>
> /* save the real devargs_list, it is restored at the end of the test */
> save_devargs_list = devargs_list;
> @@ -109,6 +110,24 @@ test_devargs(void)
> goto fail;
> free_devargs_list();
>
> + 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,bus=pci", &devargs_stack) != 0) {
> + printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
> + goto fail;
> + }
> + devargs = TAILQ_FIRST(&devargs_list);
> + if (strcmp(devargs->name, "08:00.1") != 0)
> + goto fail;
> + if (strcmp(devargs->busname, "pci") != 0)
> + goto fail;
> + if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
> + goto fail;
> +
> + free_devargs_list();
> devargs_list = save_devargs_list;
> return 0;
>
> --
> 2.13.2
>
On Thu, Jul 13, 2017 at 9:40 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> I still disagree with parsing the body of devargs to retrieve bus
> information. They should not be mixed.
>
I agree. I always said that it should be passed explicitly. If we
agree on this I'll fix the signature of rte_eal_devargs_parse()
accordingly.
> This conceptual discrepancy can be seen plainly in the code structure:
> you emulate librte_kvargs because you do not want to depend on it within
> the EAL, but copy its behavior and are forced to rewrite it partially.
>
> Can you justify this? The commitlog does not describe the problem being
> solved.
>
> I think there should be a discussion about the future format of device
> declarations. One version has been integrated in RC1 but it will be reworked
> anyway. I'd like to hear more opinions.
>
> On Tue, Jul 11, 2017 at 07:25:08PM -0400, Jan Blunck wrote:
>> Let the rte_eal_devargs_parse() function parse the "bus=" argument.
>>
>> Signed-off-by: Jan Blunck <jblunck@infradead.org>
>> ---
>> lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
>> test/test/test_devargs.c | 19 +++++
>> 2 files changed, 102 insertions(+), 48 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
>> index 2bdee9a30..a0d0e6e91 100644
>> --- a/lib/librte_eal/common/eal_common_devargs.c
>> +++ b/lib/librte_eal/common/eal_common_devargs.c
>> @@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>> return 0;
>> }
>>
>> +/*
>> + * Parse "bus=" argument without adding a dependency on rte_kvargs.h
>> + */
>> +static char *parse_bus_arg(const char *args)
>> +{
>> + const char *c;
>> + char *str = NULL;
>> +
>> + if (!args || args[0] == '\0')
>> + return NULL;
>> +
>> + c = args;
>> +
>> + do {
>> + if (strncmp(c, "bus=", 4) == 0) {
>> + c += 4;
>> + break;
>> + }
>> +
>> + c = strchr(c, ',');
>> + if (c)
>> + c++;
>> + } while (c);
>> +
>> + if (c) {
>> + str = strdup(c);
>> + c = strchr(str, ',');
>> + if (c)
>> + c = '\0';
>> + }
>> +
>> + return str;
>> +}
>> +
>> static int
>> devargs_add(const char *busname, const char *name, const char *args)
>> {
>> @@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
>> return -1;
>> }
>>
>> -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)
>> {
>> - struct rte_bus *bus = NULL;
>> - const char *devname;
>> - const size_t maxlen = sizeof(da->name);
>> - size_t i;
>> + char *busname;
>> + char *devname;
>> + char *drvargs;
>> + int ret;
>>
>> if (dev == NULL || da == NULL)
>> return -EINVAL;
>> - /* Retrieve eventual bus info */
>> - do {
>> - devname = dev;
>> - bus = rte_bus_find(bus, bus_name_cmp, dev);
>> - if (bus == NULL)
>> - break;
>> - devname = dev + strlen(bus->name) + 1;
>> - if (rte_bus_find_by_device_name(devname) == bus)
>> - break;
>> - } while (1);
>> - /* Store device name */
>> - i = 0;
>> - while (devname[i] != '\0' && devname[i] != ',') {
>> - da->name[i] = devname[i];
>> - i++;
>> - if (i == maxlen) {
>> - fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
>> - dev, maxlen);
>> - da->name[i - 1] = '\0';
>> - return -EINVAL;
>> - }
>> - }
>> - da->name[i] = '\0';
>> - if (bus == NULL) {
>> - bus = rte_bus_find_by_device_name(da->name);
>> +
>> + ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
>> + if (ret != 0)
>> + return ret;
>> +
>> + RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
>> + __FUNCTION__, devname, drvargs);
>> +
>> + /* Retrieve eventual bus info (...,bus=...)*/
>> + busname = parse_bus_arg(drvargs);
>> + if (busname == NULL) {
>> + struct rte_bus *bus;
>> +
>> + bus = rte_bus_find_by_device_name(devname);
>> if (bus == NULL) {
>> - fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
>> - da->name);
>> - return -EFAULT;
>> + RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
>> + devname);
>> + ret = -EINVAL;
>> + goto fail;
>> }
>> +
>> + busname = strdup(bus->name);
>> }
>> - da->bus = bus;
>> - /* Parse eventual device arguments */
>> - if (devname[i] == ',')
>> - da->args = strdup(&devname[i + 1]);
>> - else
>> - da->args = strdup("");
>> - if (da->args == NULL) {
>> - fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
>> - return -ENOMEM;
>> +
>> + ret = devargs_add(busname, devname, drvargs);
>> + if (ret != 0) {
>> + RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
>> + devname);
>> + goto fail;
>> }
>> - return 0;
>> +
>> + ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
>> + if (ret < 0 || ret >= (int)sizeof(da->busname))
>> + goto fail;
>> +
>> + ret = snprintf(da->name, sizeof(da->name), "%s", devname);
>> + if (ret < 0 || ret >= (int)sizeof(da->name))
>> + goto fail;
>> +
>> + da->args = strdup(drvargs);
>> + if (da->args == NULL)
>> + ret = -ENOMEM;
>> + ret = 0;
>> +
>> +fail:
>> + free(busname);
>> + free(devname);
>> + free(drvargs);
>> + return ret;
>> }
>>
>> static const struct rte_bus_conf BUS_CONF_WHITELIST = {
>> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
>> index 048b3d00b..3a1033ca3 100644
>> --- a/test/test/test_devargs.c
>> +++ b/test/test/test_devargs.c
>> @@ -58,6 +58,7 @@ test_devargs(void)
>> {
>> struct rte_devargs_list save_devargs_list;
>> struct rte_devargs *devargs;
>> + struct rte_devargs devargs_stack;
>>
>> /* save the real devargs_list, it is restored at the end of the test */
>> save_devargs_list = devargs_list;
>> @@ -109,6 +110,24 @@ test_devargs(void)
>> goto fail;
>> free_devargs_list();
>>
>> + 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,bus=pci", &devargs_stack) != 0) {
>> + printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
>> + goto fail;
>> + }
>> + devargs = TAILQ_FIRST(&devargs_list);
>> + if (strcmp(devargs->name, "08:00.1") != 0)
>> + goto fail;
>> + if (strcmp(devargs->busname, "pci") != 0)
>> + goto fail;
>> + if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
>> + goto fail;
>> +
>> + free_devargs_list();
>> devargs_list = save_devargs_list;
>> return 0;
>>
>> --
>> 2.13.2
>>
>
> --
> Gaėtan Rivet
> 6WIND
@@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
return 0;
}
+/*
+ * Parse "bus=" argument without adding a dependency on rte_kvargs.h
+ */
+static char *parse_bus_arg(const char *args)
+{
+ const char *c;
+ char *str = NULL;
+
+ if (!args || args[0] == '\0')
+ return NULL;
+
+ c = args;
+
+ do {
+ if (strncmp(c, "bus=", 4) == 0) {
+ c += 4;
+ break;
+ }
+
+ c = strchr(c, ',');
+ if (c)
+ c++;
+ } while (c);
+
+ if (c) {
+ str = strdup(c);
+ c = strchr(str, ',');
+ if (c)
+ c = '\0';
+ }
+
+ return str;
+}
+
static int
devargs_add(const char *busname, const char *name, const char *args)
{
@@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
return -1;
}
-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)
{
- struct rte_bus *bus = NULL;
- const char *devname;
- const size_t maxlen = sizeof(da->name);
- size_t i;
+ char *busname;
+ char *devname;
+ char *drvargs;
+ int ret;
if (dev == NULL || da == NULL)
return -EINVAL;
- /* Retrieve eventual bus info */
- do {
- devname = dev;
- bus = rte_bus_find(bus, bus_name_cmp, dev);
- if (bus == NULL)
- break;
- devname = dev + strlen(bus->name) + 1;
- if (rte_bus_find_by_device_name(devname) == bus)
- break;
- } while (1);
- /* Store device name */
- i = 0;
- while (devname[i] != '\0' && devname[i] != ',') {
- da->name[i] = devname[i];
- i++;
- if (i == maxlen) {
- fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
- dev, maxlen);
- da->name[i - 1] = '\0';
- return -EINVAL;
- }
- }
- da->name[i] = '\0';
- if (bus == NULL) {
- bus = rte_bus_find_by_device_name(da->name);
+
+ ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
+ if (ret != 0)
+ return ret;
+
+ RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
+ __FUNCTION__, devname, drvargs);
+
+ /* Retrieve eventual bus info (...,bus=...)*/
+ busname = parse_bus_arg(drvargs);
+ if (busname == NULL) {
+ struct rte_bus *bus;
+
+ bus = rte_bus_find_by_device_name(devname);
if (bus == NULL) {
- fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
- da->name);
- return -EFAULT;
+ RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
+ devname);
+ ret = -EINVAL;
+ goto fail;
}
+
+ busname = strdup(bus->name);
}
- da->bus = bus;
- /* Parse eventual device arguments */
- if (devname[i] == ',')
- da->args = strdup(&devname[i + 1]);
- else
- da->args = strdup("");
- if (da->args == NULL) {
- fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
- return -ENOMEM;
+
+ ret = devargs_add(busname, devname, drvargs);
+ if (ret != 0) {
+ RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
+ devname);
+ goto fail;
}
- return 0;
+
+ ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
+ if (ret < 0 || ret >= (int)sizeof(da->busname))
+ goto fail;
+
+ ret = snprintf(da->name, sizeof(da->name), "%s", devname);
+ if (ret < 0 || ret >= (int)sizeof(da->name))
+ goto fail;
+
+ da->args = strdup(drvargs);
+ if (da->args == NULL)
+ ret = -ENOMEM;
+ ret = 0;
+
+fail:
+ free(busname);
+ free(devname);
+ free(drvargs);
+ return ret;
}
static const struct rte_bus_conf BUS_CONF_WHITELIST = {
@@ -58,6 +58,7 @@ test_devargs(void)
{
struct rte_devargs_list save_devargs_list;
struct rte_devargs *devargs;
+ struct rte_devargs devargs_stack;
/* save the real devargs_list, it is restored at the end of the test */
save_devargs_list = devargs_list;
@@ -109,6 +110,24 @@ test_devargs(void)
goto fail;
free_devargs_list();
+ 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,bus=pci", &devargs_stack) != 0) {
+ printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
+ goto fail;
+ }
+ devargs = TAILQ_FIRST(&devargs_list);
+ if (strcmp(devargs->name, "08:00.1") != 0)
+ goto fail;
+ if (strcmp(devargs->busname, "pci") != 0)
+ goto fail;
+ if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
+ goto fail;
+
+ free_devargs_list();
devargs_list = save_devargs_list;
return 0;