[dpdk-dev] [PATCH v10 08/27] devargs: add function to parse device layers

[Gaëtan Rivet]

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