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

Wiles, Keith keith.wiles at intel.com
Wed Jun 28 01:46:48 CEST 2017


> 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?

> +{
> +	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?

> +	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?

> +			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?

> +	}
> +	/* 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?

> +			da->name[i-1] = '\0’;

I believe the must have spaces around the - e.g. [i - 1]

> +			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



More information about the dev mailing list