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