[dpdk-dev,v5,16/19] devargs: introduce cleaner parsing helper
Checks
Commit Message
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@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(-)
Comments
> On Jun 20, 2017, at 4:35 PM, Gaetan Rivet <gaetan.rivet@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@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
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@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@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
>
@@ -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``
@@ -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)
+{
+ struct rte_bus *bus;
+ const char *c;
+ const size_t maxlen = sizeof(da->name);
+ size_t i;
+
+ if ((dev) == NULL || (da) == NULL)
+ return -EINVAL;
+ 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");
+ bus = NULL;
+ } else if (rte_bus_from_dev(dev)) {
+ /* false positive on bus name. */
+ bus = NULL;
+ } else {
+ c = &dev[i+1];
+ }
+ }
+ /* 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);
+ da->name[i-1] = '\0';
+ 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);
+ return -EFAULT;
+ }
+ }
+ da->bus = bus;
+ /* Parse eventual device arguments */
+ if (c[i] == ',')
+ da->args = strdup(&c[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);
@@ -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