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