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

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Legacy, Allain [mailto:allain.leg...@windriver.com]
> Sent: Friday, March 3, 2017 11:31 AM
> To: Dumitrescu, Cristian <cristian.dumitre...@intel.com>; Yuanhan Liu
> <yuanhan....@linux.intel.com>; Richardson, Bruce
> <bruce.richard...@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolli...@windriver.com>
> Subject: RE: [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character
> 
> > -----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);
> }


Regardless of the approaches 1. or 2., we must control the acceptable set of 
chars for the comment separator, and the way to do this is through enum/flags.

Both approaches can support this. Therefore, IMO the separator char is not 
enough to justify approach 1. I would only go for approach 1 if there are some 
other parameters that we could consider adding to the load function now or 
later. Do you see any?