From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=5dVarlwohgsVQzheyBBLP9IWSCwp3CCN2jIen+W6kVg=; b=tsVem3MK4G45B+sp3gI5rw/VxRS7u/dBVDasKt4RIiyYPn7RCZ4YwKQ5JzIRLwpFb+ CYxor3+SL9B6JFX5B9eOzFHcq0xKWGsO6T+DCWb9o6jyjGsxccXgEsfw7ZGV5xTrn5M9 W8g9VGaeQ1vTMnPknpxDwEaTHEBH/Aq2qVpNyMAsGR8Teimj5I8pVB7hwFHRtnwywkFJ elXJZmA15GBnMyNi+Zp8TKXfw+WurcIo8MZ9A5KLz088nPo9r/SSBPNi9pQcxbL/jzyq jW2VDqtRYz2hse4ydMGen/UOMy5NDCSeaf+UerkcGPrrLpdDx13vVLuCBYy/aDz34jm4 RVyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=5dVarlwohgsVQzheyBBLP9IWSCwp3CCN2jIen+W6kVg=; b=U9MGB3owsaZO6fDEt7A6uait/HHEInKj752C3OXC9LNya9QQDuVb2sj136WnTMr2xL nCWuDnCmbmFJOHzA4xQlyjIWCtjwjqXr7fj4tb4FxKFrD2l9nYeewAo0rmHKKuc4nWfZ hbnk8kkF3i4YokOC+tJ4WqNwxFDuPfGDqo0vfYJO42G97fK3umnnqHOgKgD/yzo9okg7 XUnnp3EKnV2gzIdYcOohaJ/KnRogaspPk7Kjd4wRHrC1TEmxOCuyJLVkfS6aXzUqPdwd L0nHn9cfcUCbdGTIeBXvrCh1F60AGWG+mX6bsYSoMRIQlHVFeLxfyJV6yznIQs4yIQqh Io7w== X-Gm-Message-State: AIVw1125nyjkwOUeNfKL7BTqdAVNIUcdMCYDwwAqYhF053TdkyN8f2Hx 51NM4E9x7ehBvf8/ X-Received: by 10.28.208.69 with SMTP id h66mr2236822wmg.66.1499953211981; Thu, 13 Jul 2017 06:40:11 -0700 (PDT) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 31sm4055164wrd.20.2017.07.13.06.40.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 13 Jul 2017 06:40:11 -0700 (PDT) Date: Thu, 13 Jul 2017 15:40:03 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Jan Blunck Cc: dev@dpdk.org 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-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170711232512.54641-10-jblunck@infradead.org> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH 09/13] devargs: parse "bus=" argument X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 Jul 2017 13:40:12 -0000 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