Re: [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character

[Bruce Richardson]

On Fri, Mar 03, 2017 at 11:31:11AM +0000, Legacy, Allain wrote:
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitre...@intel.com]
> > Possible options that I see:
> > 1. Add a new parameters argument to the load functions (e.g. struct
> > cfgfile_params *p), whit the comment char as one (and currently only) field
> > of this struct. Drawbacks: API change that might have to be announced one
> > release before the actual API change.
> 
> I would prefer this option as it provides more flexibility.  We can leave the 
> existing API as is and a wrapper that accepts additional parameters.   
> Something like the following (with implementations in the .c obviously rather 
> than inline the header like I have it here).  There are several examples of 
> this pattern already in the dpdk (i.e., ring APIs, mempool APIs, etc.) where 
> we use a common function invoked by higher level functions that pass in 
> additional parameters to customize behavior.
> 
> struct rte_cfgfile *_rte_cfgfile_load(const char *filename,
>                                           const struct rte_cfgfile_params 
> *params);
> 
> struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags)
> {
>         struct rte_cfgfile_params params;
> 
>         rte_cfgfile_set_default_params(&params);
>         params |= flags;
>         return _rte_cfgfile_load(filename, &params);
> }
> 
> struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
>                                                     const struct 
> rte_cfgfile_params *params)
> {
>         return _rte_cfgfile_load(filename, params);
> }

No need for a new API. Just add the extra parameter to the existing load
parameter and use function versioning for ABI compatilibity. Since it's
only one function, I don't think using versioning is a big deal, and
that's what it is there for.

Also, for a single parameter like a comment char, I don't think we need
to go creating a separate structure. The current flags parameter is
unused, so just replace it with the comment char one. With using the
structure, any additions to the struct would be an ABI change anyway, so
I see little point in using it, unless we already know of additional
parameters we will be adding in future. [It's an ABI change even when
adding to the end, since the struct is allocated in the app itself, not
the library.]

/Bruce