[dpdk-dev] [PATCH v5 16/19] devargs: introduce cleaner parsing helper

[Gaëtan Rivet]

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 <gaetan.rivet at 6wind.com> 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 <gaetan.rivet at 6wind.com>
> > ---
> > 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