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

  The current cfgfile comment character is hardcoded to ';'.  This commit
introduces a configuration attribute to allow an application to select a
different character.  This is to ease adoption by applications that have an
existing configuration file which may use a different comment character.
For instance, an application may already have a configuration file that
uses the '#' as the comment character.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 config/common_base               | 1 +
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
  

On Thu, Mar 02, 2017 at 02:29:27PM -0500, Allain Legacy wrote:
> The current cfgfile comment character is hardcoded to ';'.  This commit
> introduces a configuration attribute to allow an application to select a
> different character.  This is to ease adoption by applications that have an
> existing configuration file which may use a different comment character.
> For instance, an application may already have a configuration file that
> uses the '#' as the comment character.
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  config/common_base               | 1 +
>  lib/librte_cfgfile/rte_cfgfile.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/config/common_base b/config/common_base
> index aeee13e..32a42d7 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -477,6 +477,7 @@ CONFIG_RTE_LIBRTE_TIMER_DEBUG=n
>  # Compile librte_cfgfile
>  #
>  CONFIG_RTE_LIBRTE_CFGFILE=y
> +CONFIG_RTE_LIBRTE_CFGFILE_COMMENT_CHAR=';'
>  

We are trying to avoid adding in extra build-time options to DPDK, so
can you please rework this patch to make it a run-time option instead.
Perhaps just add a set_comment_char() API call to the library. If this
is a setting per cfgfile instance it would then allow applications to
simultaneously use files with different formats. For example, if in
future we use cfgfile library to configure DPDK EAL, a preexisting file
for that shipped with DPDK may conflict in format with an
application-specific one.

	/Bruce
  

> -----Original Message-----
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> We are trying to avoid adding in extra build-time options to DPDK, so
> can you please rework this patch to make it a run-time option instead.
> Perhaps just add a set_comment_char() API call to the library. If this
> is a setting per cfgfile instance it would then allow applications to
> simultaneously use files with different formats. For example, if in
> future we use cfgfile library to configure DPDK EAL, a preexisting file
> for that shipped with DPDK may conflict in format with an
> application-specific one.

Ok, sounds reasonable.
  

On Thu, Mar 02, 2017 at 09:10:15PM +0000, Bruce Richardson wrote:
> On Thu, Mar 02, 2017 at 02:29:27PM -0500, Allain Legacy wrote:
> > The current cfgfile comment character is hardcoded to ';'.  This commit
> > introduces a configuration attribute to allow an application to select a
> > different character.  This is to ease adoption by applications that have an
> > existing configuration file which may use a different comment character.
> > For instance, an application may already have a configuration file that
> > uses the '#' as the comment character.
> > 
> > Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> > ---
> >  config/common_base               | 1 +
> >  lib/librte_cfgfile/rte_cfgfile.c | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/config/common_base b/config/common_base
> > index aeee13e..32a42d7 100644
> > --- a/config/common_base
> > +++ b/config/common_base
> > @@ -477,6 +477,7 @@ CONFIG_RTE_LIBRTE_TIMER_DEBUG=n
> >  # Compile librte_cfgfile
> >  #
> >  CONFIG_RTE_LIBRTE_CFGFILE=y
> > +CONFIG_RTE_LIBRTE_CFGFILE_COMMENT_CHAR=';'
> >  
> 
> We are trying to avoid adding in extra build-time options to DPDK, so
> can you please rework this patch to make it a run-time option instead.

+1 for making it a run-time option.

	--yliu

> Perhaps just add a set_comment_char() API call to the library. If this
> is a setting per cfgfile instance it would then allow applications to
> simultaneously use files with different formats. For example, if in
> future we use cfgfile library to configure DPDK EAL, a preexisting file
> for that shipped with DPDK may conflict in format with an
> application-specific one.
> 
> 	/Bruce
  

> > We are trying to avoid adding in extra build-time options to DPDK, so
> > can you please rework this patch to make it a run-time option instead.
> 
> +1 for making it a run-time option.
> 
> 	--yliu
> 

+1

> > Perhaps just add a set_comment_char() API call to the library. If this
> > is a setting per cfgfile instance it would then allow applications to
> > simultaneously use files with different formats. For example, if in
> > future we use cfgfile library to configure DPDK EAL, a preexisting file
> > for that shipped with DPDK may conflict in format with an
> > application-specific one.
> >

I don't think this approach will work well for the current implementation, as the config file load function simply complete the parsing on the file in a single step, so any update of the comment char after the load function will not produce any effects.

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.
2. Use the flags argument. Basically define a new flag value for each acceptable comment char. It does not require an API change, it has the advantage of limiting the characters that can be accepted as comments (as we probably do not want to allow e.g. letters, digits, tab, space, unprintable chars, etc here). I am OK with adding all the commonly used single character comment separators ("; # % @ !"), with the default as the current option of ";"

My vote will be to go for option 2. Naming convention proposal:
RTE_CFGFILE_COMMENT_SEPARATOR_CHR33; /**< ! */
RTE_CFGFILE_COMMENT_SEPARATOR_CHR35; /**< # */
RTE_CFGFILE_COMMENT_SEPARATOR_CHR37; /**< % */
RTE_CFGFILE_COMMENT_SEPARATOR_CHR59; /**< ; */
RTE_CFGFILE_COMMENT_SEPARATOR_CHR64; /**< @ */
Of course, the flags argument needs to be checked for the presence of a single separator char flag.

Regards,
Cristian
  

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@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);
}
  

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

On Fri, Mar 03, 2017 at 11:31:11AM +0000, Legacy, Allain wrote:
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@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
  

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
 > 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?

No, I don't have any future parameters in mind but that doesn't mean that none will arise eventually.   IMO, the comment character should be specified as an actual "char" in the rte_cfgfile_params.  Specifying it as a flag is a bit kludgy - I don't like overloading a flag/enum to specify something that already has a type that can be used (char).   Also, I don't think we need to control which comment characters are valid.  If the app wants to use a 'X' then that's their choice.
  

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Friday, March 3, 2017 12:14 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolliffe@windriver.com>
> Subject: RE: [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character
> 
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
>  > 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?
> 
> No, I don't have any future parameters in mind but that doesn't mean that
> none will arise eventually.   IMO, the comment character should be specified
> as an actual "char" in the rte_cfgfile_params.  Specifying it as a flag is a bit
> kludgy - I don't like overloading a flag/enum to specify something that
> already has a type that can be used (char).   Also, I don't think we need to
> control which comment characters are valid.  If the app wants to use a 'X'
> then that's their choice.
> 
> 

I disagree here. I think we must control the set of allowed separators to avoid confusion.
  

> -----Original Message-----
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
 > 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
In my earlier patch, I proprose using a "global" flag to indicate that an unnamed section exists so the flags argument would still be needed.  

> 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.
We already have 2 parameters in mind - flags, and comment char.  I don't feel that combining the two in a single enum is particularly good since it would be better to allow the application the freedom to set an arbitrary comment character and not be locked in to any static list that we choose (see my previous email response).
  

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> I disagree here. I think we must control the set of allowed separators to
> avoid confusion.
I don't understand.  What will be confusing?  The app owns the file format and is responsible to ensure that it is parseable.
  

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Friday, March 3, 2017 12:19 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolliffe@windriver.com>
> Subject: RE: [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> > I disagree here. I think we must control the set of allowed separators to
> > avoid confusion.
> I don't understand.  What will be confusing?  The app owns the file format
> and is responsible to ensure that it is parseable.
> 

I does not make sense to allow letters, numbers, formatting characters (tabs, lf, cr, etc), unprintable characters, etc as comment separators. In fact, if you look on the ASCII set, there are about 5 chars out of 256 that can be used as comment separators (the ones I listed earlier). The others are not suitable as comment separators, so none of the commonly used parsers allow them. The API should not allow options that do not make sense.
  

On Fri, Mar 03, 2017 at 12:17:47PM +0000, Legacy, Allain wrote:
> > -----Original Message-----
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
>  > 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
> In my earlier patch, I proprose using a "global" flag to indicate that an unnamed section exists so the flags argument would still be needed.

Ok, good point, I missed that.

> 
> > 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.
> We already have 2 parameters in mind - flags, and comment char.  I don't feel that combining the two in a single enum is particularly good since it would be better to allow the application the freedom to set an arbitrary comment character and not be locked in to any static list that we choose (see my previous email response).
>
I also agree on not using enums and not limiting comment chars.

I don't particularly like config structs, and would prefer individual
flags and comment char parameters - given it's not a huge list of
params, just 2 - but no big deal either way.

/Bruce