[dpdk-dev] [PATCH 09/13] devargs: parse "bus=" argument

[Jan Blunck]

On Thu, Jul 13, 2017 at 9:40 AM, Gaëtan Rivet <gaetan.rivet at 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 at 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