Hi Thomas,
On Wed, Jul 11, 2018 at 10:19:07AM +0200, Thomas Monjalon wrote:
> 05/07/2018 13:48, Gaetan Rivet:
> > +/**
> > + * @internal
> > + * Parse a device string and store its information in an
> > + * rte_devargs structure.
>
> Please, explain what is a layer.
>
> > + *
> > + * Note: if the "data" field of da points to devstr,
>
> Better to use "devargs" as variable name, instead of "da".
>
> > + * then no dynamic allocation is performed and the rte_devargs
> > + * can be safely discarded.
> > + *
> > + * Otherwise ``data`` will hold a workable copy of devstr, that will be
> > + * used by layers descriptors within rte_devargs. In this case,
> > + * any rte_devargs should be cleaned-up before being freed.
> > + *
> > + * @param da
> > + * rte_devargs structure to fill.
> > + *
> > + * @param devstr
> > + * Device string.
> > + *
> > + * @return
> > + * 0 on success.
> > + * Negative errno values on error (rte_errno is set).
> > + */
> > +int
> > +rte_devargs_layers_parse(struct rte_devargs *da,
> > + const char *devstr);
> > +
> > #endif /* _EAL_PRIVATE_H_ */
> > diff --git a/lib/librte_eal/common/include/rte_devargs.h
> > b/lib/librte_eal/common/include/rte_devargs.h
> > index 6c3b6326b..148600258 100644
> > --- a/lib/librte_eal/common/include/rte_devargs.h
> > +++ b/lib/librte_eal/common/include/rte_devargs.h
> > @@ -51,12 +51,19 @@ struct rte_devargs {
> > enum rte_devtype type;
> > /** Device policy. */
> > enum rte_dev_policy policy;
> > - /** Bus handle for the device. */
> > - struct rte_bus *bus;
> > /** Name of the device. */
> > char name[RTE_DEV_NAME_MAX_LEN];
> > + RTE_STD_C11
> > + union {
> > /** Arguments string as given by user or "" for no argument. */
> > - char *args;
> > + char *args;
> > + const char *drvstr;
> > + };
> > + struct rte_bus *bus; /**< bus handle. */
> > + struct rte_class *cls; /**< class handle. */
>
> "class" is more readable than "cls"
>
I was thinking that maybe this could be a problem when trying to build
with C++. The EAL headers in DPDK are meant to be included in C++ apps,
I think ``class`` would be an issue then.
If I'm mistaken, then sure, class is a better name.
> > + const char *busstr; /**< bus-related part of device string. */
>
> bus_str ?
>
> > + const char *clsstr; /**< bus-related part of device string. */
>
> class_str ?
> + there is a typo in the comment (copy/pasted "bus")
>
> > + const char *data; /**< Device string storage. */
> > };
>
>
>
Otherwise, ok with the rest of the comments, will fix (as well as the
previous emails). Thanks for reading.
--
Gaëtan Rivet
6WIND
