From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH v10 08/27] devargs: add function to parse device layers Date: Wed, 11 Jul 2018 10:41:16 +0200 Message-ID: <20180711084116.dbbou4vbbp2y5tcn@bidouze.vm.6wind.com> References: <0a2d77e22658987a6e1e650913f020a140c2c1a9.1530791217.git.gaetan.rivet@6wind.com> <2914659.2M3dPKUJVY@xps> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org To: Thomas Monjalon Return-path: Received: from mail-wm0-f67.google.com (mail-wm0-f67.google.com [74.125.82.67]) by dpdk.org (Postfix) with ESMTP id BB5561B4FD for ; Wed, 11 Jul 2018 10:41:33 +0200 (CEST) Received: by mail-wm0-f67.google.com with SMTP id v3-v6so1646938wmh.0 for ; Wed, 11 Jul 2018 01:41:33 -0700 (PDT) Content-Disposition: inline In-Reply-To: <2914659.2M3dPKUJVY@xps> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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