From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH 09/13] devargs: parse "bus=" argument Date: Thu, 13 Jul 2017 15:40:03 +0200 Message-ID: <20170713134003.GK11154@bidouze.vm.6wind.com> References: <20170711232512.54641-1-jblunck@infradead.org> <20170711232512.54641-10-jblunck@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: Jan Blunck Return-path: Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) by dpdk.org (Postfix) with ESMTP id 6ED7229D9 for ; Thu, 13 Jul 2017 15:40:12 +0200 (CEST) Received: by mail-wm0-f47.google.com with SMTP id 62so23997820wmw.1 for ; Thu, 13 Jul 2017 06:40:12 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20170711232512.54641-10-jblunck@infradead.org> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > --- > 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