From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f174.google.com (mail-wr0-f174.google.com [209.85.128.174]) by dpdk.org (Postfix) with ESMTP id 4304E2C8 for ; Tue, 4 Jul 2017 23:50:20 +0200 (CEST) Received: by mail-wr0-f174.google.com with SMTP id k67so253691859wrc.2 for ; Tue, 04 Jul 2017 14:50:20 -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=/7RjRLQ3UgMH01zWb1i3+xJi4cosajSUB+/hyyH2WXM=; b=woGplkMKV2jEeksSZ5NvD6hsiXY6YJx0RXxiKIHLYT+x55zFFdeTLipTz7nYka7c9+ AonPgDpOkBgCxZtCdbKXKOBkKsCvWrUPhlCQ/0KgkPE6Ee9nXF5haQrVlls5Pk6Z75IJ 8u4X8T15wZQfD1qYG2qkcDq0IMvELHeIPspUtEw1pdYZGB3MZRIV8Ld1W84qiN1Xidyq gSW+9Ye3JC6tSDqluWp1ToerF1QMFe5mbSvbFULt60PuteoNY05wqi47PWhHwQac0hP5 oUewERnwFIfKpoHXvlJ6GW8QvfW5qh16jO2oSRrbL+lkSOAKhnMUjC4JtL8V27Oha70y pGvA== 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=/7RjRLQ3UgMH01zWb1i3+xJi4cosajSUB+/hyyH2WXM=; b=WO8JWCJCMunxjghYf73PJ0cpbGkjbz5/c30Qkn+TDlu3VvFgxo+xFLOcbX2xhr5Uaj neUDsnR5h+iwr/f1AOQ5TLF+B5Uj1G2b+absE1GUT0QeqemL+cHKiObON6nmxemQ/lRc AmDszdO/5rejq00oS38UlL6GTBGj5EWNwgtqcxQy+BPa7+VEk0KAUs6l4UvWVt0ZA4La USL7a9H3noQDgH8M/QH7EO7WHnOvtHsileusO7gsXgiQ486ZY/x0Z5oPuZ+kiVBce+8d E9+60OWiJ7VLkYCSMAjWKnkNZDDUMXz00Nrc9q+r1SWElmkr6CWjSWT71RoL9EG7k44n luug== X-Gm-Message-State: AKS2vOzdusAzf6RmegmmbgaBLudVXV2P6D5RZJnrsFrK9Aj59Upp4cBe 1VAZw9CLBSo1cE/m X-Received: by 10.223.141.138 with SMTP id o10mr42236469wrb.69.1499205019657; Tue, 04 Jul 2017 14:50:19 -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 y2sm7612288wmy.29.2017.07.04.14.50.18 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 04 Jul 2017 14:50:18 -0700 (PDT) Date: Tue, 4 Jul 2017 23:50:10 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: "Wiles, Keith" Cc: "dev@dpdk.org" Message-ID: <20170704215010.GD11154@bidouze.vm.6wind.com> References: <7BAD764D-F3B7-4426-B820-B493CE9385DA@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <7BAD764D-F3B7-4426-B820-B493CE9385DA@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v5 16/19] devargs: introduce cleaner parsing helper 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: Tue, 04 Jul 2017 21:50:20 -0000 Hi Keith, Thanks for the review. On Tue, Jun 27, 2017 at 11:46:48PM +0000, Wiles, Keith wrote: > > > On Jun 20, 2017, at 4:35 PM, Gaetan Rivet wrote: > > > > Introduce a more versatile helper to parse device strings. This > > helper expects a generic rte_devargs structure as storage in order not > > to require any API changes in the future, should this structure be > > updated. > > > > The old equivalent function is thus being deprecated, as its API does > > not allow to accompany current rte_devargs evolutions. > > > > A deprecation notice is issued. > > > > This new helper will parse bus information as well as device name and > > device parameters. It does not allocate an rte_devargs structure and > > expects one to be given as input. > > > > Signed-off-by: Gaetan Rivet > > --- > > doc/guides/rel_notes/deprecation.rst | 5 ++ > > lib/librte_eal/common/eal_common_devargs.c | 91 ++++++++++++++++++++--------- > > lib/librte_eal/common/include/rte_devargs.h | 20 +++++++ > > 3 files changed, 90 insertions(+), 26 deletions(-) > > > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst > > index 1786a59..fb95ced 100644 > > --- a/doc/guides/rel_notes/deprecation.rst > > +++ b/doc/guides/rel_notes/deprecation.rst > > @@ -105,3 +105,8 @@ Deprecation Notices > > The non-"do-sig" versions of the hash tables will be removed > > (including the ``signature_offset`` parameter) > > and the "do-sig" versions renamed accordingly. > > + > > +* eal: the following function is deprecated starting from 17.08 and will > > + be removed in 17.11: > > + > > + - ``rte_eal_parse_devargs_str``, replaced by ``rte_eal_devargs_parse`` > > diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c > > index 321a62d..f2e11f9 100644 > > --- a/lib/librte_eal/common/eal_common_devargs.c > > +++ b/lib/librte_eal/common/eal_common_devargs.c > > @@ -77,6 +77,66 @@ rte_eal_parse_devargs_str(const char *devargs_str, > > return 0; > > } > > > > +int > > +rte_eal_devargs_parse(const char *dev, > > + struct rte_devargs *da) > > Does this line need to be broken into two lines? > Not really, will change. > > +{ > > + struct rte_bus *bus; > > + const char *c; > > + const size_t maxlen = sizeof(da->name); > > + size_t i; > > + > > + if ((dev) == NULL || (da) == NULL) > > + return -EINVAL; > > Why have () around these variables and I think the normal method is ‘if (!dev || !da) …’ is that preferred method? > No reason for the additional parenthesis. Otherwise, the explicit test against NULL is preferred in the guideline. > > + c = dev; > > + /* Retrieve eventual bus info */ > > + bus = rte_bus_from_name(dev); > > + if (bus) { > > + i = strlen(bus->name); > > + if (dev[i] == '\0') { > > + fprintf(stderr, "WARNING: device name matches a bus name.\n”); > > At this point has the RTE_LOG() system been inited? > This helper is typically called very early at init. Right underneath, rte_eal_devargs_add also avoids using RTE_LOG. It may be a mistake, I must look into it. > > + bus = NULL; > > + } else if (rte_bus_from_dev(dev)) { > > + /* false positive on bus name. */ > > + bus = NULL; > > + } else { > > + c = &dev[i+1]; > > + } > > Single line if/else statements do not use the “{}” around the one line. I believe this is the default rule. Does it count for the 'else if' above it too? > Right. > > + } > > + /* Store device name */ > > + i = 0; > > + while (c[i] != '\0' && c[i] != ',') { > > + da->name[i] = c[i]; > > + i++; > > + if (i == maxlen) { > > + fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n", > > + dev, maxlen); > > Same question here. is this line too long? > Error strings should not be cut through to allow grepping for it. > > + da->name[i-1] = '\0’; > > I believe the must have spaces around the - e.g. [i - 1] > yes > > + return -EINVAL; > > + } > > + } > > + da->name[i] = '\0'; > > + if (!bus) { > > + bus = rte_bus_from_dev(da->name); > > + if (!bus) { > > + fprintf(stderr, "ERROR: failed to parse bus info from device \"%s\"\n", > > + da->name); > > Same here. > > > + return -EFAULT; > > + } > > + } > > + da->bus = bus; > > + /* Parse eventual device arguments */ > > + if (c[i] == ',') > > + da->args = strdup(&c[i+1]); > [i + 1] > > > + else > > + da->args = strdup(""); > > + if (da->args == NULL) { > > + fprintf(stderr, "ERROR: not enough memory to parse arguments\n"); > > + return -ENOMEM; > > + } > > + return 0; > > +} > > + > > /* store a whitelist parameter for later parsing */ > > int > > rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > > @@ -84,35 +144,16 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > > struct rte_devargs *devargs = NULL; > > const char *dev = devargs_str; > > struct rte_bus *bus; > > - char *buf = NULL; > > - int ret; > > > > - /* use malloc instead of rte_malloc as it's called early at init */ > > - devargs = malloc(sizeof(*devargs)); > > + /* use calloc instead of rte_zmalloc as it's called early at init */ > > + devargs = calloc(1, sizeof(*devargs)); > > if (devargs == NULL) > > goto fail; > > > > - memset(devargs, 0, sizeof(*devargs)); > > - devargs->type = devtype; > > - > > - bus = rte_bus_from_name(dev); > > - if (bus) { > > - dev += strlen(bus->name) + 1; > > - } else { > > - bus = rte_bus_from_dev(dev); > > - if (!bus) { > > - fprintf(stderr, "ERROR: failed to parse bus info from device declaration\n"); > > - goto fail; > > - } > > - } > > - devargs->bus = bus; > > - if (rte_eal_parse_devargs_str(dev, &buf, &devargs->args)) > > - goto fail; > > - > > - /* save device name. */ > > - ret = snprintf(devargs->name, sizeof(devargs->name), "%s", buf); > > - if (ret < 0 || ret >= (int)sizeof(devargs->name)) > > + if (rte_eal_devargs_parse(dev, devargs)) > > goto fail; > > + devargs->type = devtype; > > + bus = devargs->bus; > > if (devargs->type == RTE_DEVTYPE_WHITELISTED) { > > if (bus->conf.scan_mode == RTE_BUS_SCAN_UNDEFINED) { > > bus->conf.scan_mode = RTE_BUS_SCAN_WHITELIST; > > @@ -129,12 +170,10 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) > > } > > } > > > > - free(buf); > > TAILQ_INSERT_TAIL(&devargs_list, devargs, next); > > return 0; > > > > fail: > > - free(buf); > > if (devargs) { > > free(devargs->args); > > free(devargs); > > diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h > > index 6e9e134..2ab8864 100644 > > --- a/lib/librte_eal/common/include/rte_devargs.h > > +++ b/lib/librte_eal/common/include/rte_devargs.h > > @@ -119,6 +119,26 @@ int rte_eal_parse_devargs_str(const char *devargs_str, > > char **drvname, char **drvargs); > > > > /** > > + * Parse a device string. > > + * > > + * Verify that a bus is capable of handling the device passed > > + * in argument. Store which bus will handle the device, its name > > + * and the eventual device parameters. > > + * > > + * @param dev > > + * The device declaration string. > > + * @param da > > + * The devargs structure holding the device information. > > + * > > + * @return > > + * - 0 on success. > > + * - Negative errno on error. > > + */ > > +int > > +rte_eal_devargs_parse(const char *dev, > > + struct rte_devargs *da); > > + > > +/** > > * Add a device to the user device list > > * > > * For PCI devices, the format of arguments string is "PCI_ADDR" or > > -- > > 2.1.4 > > > > Regards, > Keith > -- Gaëtan Rivet 6WIND