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