> On Jun 20, 2017, at 4:35 PM, Gaetan Rivet <gaetan.ri...@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.ri...@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
