[PATCH 0/5] librte_cfgfile enhancement

[PATCH 0/5] librte_cfgfile enhancement

[Allain Legacy]

This patchset includes some minor enhancements that we have developped for
our DPDK application.  We would like to contribute them upstream to help
ease adoption of the DPDK by anyone looking for this type of
functionality.  The commit logs on each patch should be self-sufficient in
explain the intent and purpose.

Allain Legacy (4):
  cfgfile: configurable comment character
  cfgfile: cfg object not initialized after allocation
  cfgfile: add support for unamed global section
  cfgfile: use strnlen to constrain memchr search

Joseph Richard (1):
  cfgfile: increase local buffer size for max name and value

 config/common_base               |  1 +
 lib/librte_cfgfile/rte_cfgfile.c | 28 +++++++++++++++++++++++-----
 lib/librte_cfgfile/rte_cfgfile.h |  6 ++++++
 3 files changed, 30 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] cfgfile: configurable comment character

[Allain Legacy]

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=';'
 
 #
 # Compile librte_cmdline
diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 829109a..603dd73 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -116,7 +116,7 @@ struct rte_cfgfile *
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, ';', sizeof(buffer));
+		pos = memchr(buffer, RTE_LIBRTE_CFGFILE_COMMENT_CHAR, len);
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
-- 
1.8.3.1

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

[Bruce Richardson]

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

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

[Legacy, Allain]

> -----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.

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

[Yuanhan Liu]

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

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

[Dumitrescu, Cristian]

> > 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

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

[Legacy, Allain]

> -----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);
}

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

[Dumitrescu, Cristian]

> -----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?

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

[Legacy, Allain]

> -----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.

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

[Dumitrescu, Cristian]

> -----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.

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

[Legacy, Allain]

> -----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.  

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

[Dumitrescu, Cristian]

> -----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.

Re: [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.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

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

[Legacy, Allain]

> -----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). 

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

[Bruce Richardson]

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

[PATCH 2/5] cfgfile: cfg object not initialized after allocation

[Allain Legacy]

After the call to malloc() the cfg object is only partially initialized
with memset().  If parsing of the ini file fails because of a parsing error
then the subsequent call to rte_cfgfile_close() segfaults due to
uninitialized memory.

This reproducible by attempting to parse a ini file that has a key=value
entry before the first [section] statement.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 603dd73..7a9206d 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -94,18 +94,19 @@ struct rte_cfgfile *
 	int curr_entry = -1;
 	char buffer[256] = {0};
 	int lineno = 0;
+	size_t size;
 	struct rte_cfgfile *cfg = NULL;
 
 	FILE *f = fopen(filename, "r");
 	if (f == NULL)
 		return NULL;
 
-	cfg = malloc(sizeof(*cfg) + sizeof(cfg->sections[0]) *
-		allocated_sections);
+	size = sizeof(*cfg) + sizeof(cfg->sections[0]) * allocated_sections;
+	cfg = malloc(size);
 	if (cfg == NULL)
 		goto error2;
 
-	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
+	memset(cfg, 0, size);
 
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
-- 
1.8.3.1

[PATCH 3/5] cfgfile: add support for unamed global section

[Allain Legacy]

The current implementation of the cfgfile library requires that all
key=value pairs be within [SECTION] definitions.  The ini file standard
allows for key=value pairs in an unnamed section.  That section is
considered the [GLOBAL] section.

This commit adds the capability of parsing key=value pairs from such an
unnamed section. The CFG_FLAG_GLOBAL_SECTION flag must be passed to the
rte_cfgfile_load() API to enable this functionality.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 16 ++++++++++++++++
 lib/librte_cfgfile/rte_cfgfile.h |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 7a9206d..2aba169 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -108,6 +108,22 @@ struct rte_cfgfile *
 
 	memset(cfg, 0, size);
 
+	if (flags & CFG_FLAG_GLOBAL_SECTION) {
+		curr_section = 0;
+		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
+		cfg->sections[curr_section] = malloc(
+			sizeof(*cfg->sections[0]) +
+			sizeof(cfg->sections[0]->entries[0]) *
+			allocated_entries);
+		if (cfg->sections[curr_section] == NULL) {
+			printf("Error - no memory for global section\n");
+			goto error1;
+		}
+
+		snprintf(cfg->sections[curr_section]->name,
+				 sizeof(cfg->sections[0]->name), "GLOBAL");
+	}
+
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
 		size_t len = strnlen(buffer, sizeof(buffer));
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index b40e6a1..fce1efc 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -67,6 +67,12 @@ struct rte_cfgfile_entry {
 };
 
 /**
+ * Indicates that the file supports key value entries before the first defined
+ * section.  These entries can be accessed in the "GLOBAL" section.
+ */
+#define CFG_FLAG_GLOBAL_SECTION (1 << 0)
+
+/**
 * Open config file
 *
 * @param filename
-- 
1.8.3.1

Re: [PATCH 3/5] cfgfile: add support for unamed global section

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Thursday, March 2, 2017 7:29 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolliffe@windriver.com>
> Subject: [PATCH 3/5] cfgfile: add support for unamed global section
> 
> The current implementation of the cfgfile library requires that all
> key=value pairs be within [SECTION] definitions.  The ini file standard
> allows for key=value pairs in an unnamed section.  That section is
> considered the [GLOBAL] section.
> 
> This commit adds the capability of parsing key=value pairs from such an
> unnamed section. The CFG_FLAG_GLOBAL_SECTION flag must be passed to
> the
> rte_cfgfile_load() API to enable this functionality.
> 

What is the motivation for the having key/value pair outside of any section? What would be the drawback of having the user explicitly define a GLOBAL section to host these key/value pairs?

Re: [PATCH 3/5] cfgfile: add support for unamed global section

[Legacy, Allain]

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> 
> What is the motivation for the having key/value pair outside of any section?
> What would be the drawback of having the user explicitly define a GLOBAL
> section to host these key/value pairs?
Global parameters that are outside of sections are part of the ini informal "standard".   Our app use this format.

https://en.wikipedia.org/wiki/INI_file#Global_properties

Re: [PATCH 3/5] cfgfile: add support for unamed global section

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Friday, March 3, 2017 11:04 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolliffe@windriver.com>
> Subject: RE: [PATCH 3/5] cfgfile: add support for unamed global section
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> >
> > What is the motivation for the having key/value pair outside of any
> section?
> > What would be the drawback of having the user explicitly define a GLOBAL
> > section to host these key/value pairs?
> Global parameters that are outside of sections are part of the ini informal
> "standard".   Our app use this format.
> 
> https://en.wikipedia.org/wiki/INI_file#Global_properties
> 
> 

I am not totally against it, but IMO this option is a bit confusing. Again, what's wrong with user explicitly adding the GLOBAL section if needed?

Re: [PATCH 3/5] cfgfile: add support for unamed global section

[Legacy, Allain]

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> 
> I am not totally against it, but IMO this option is a bit confusing. Again, what's
> wrong with user explicitly adding the GLOBAL section if needed?
There's nothing wrong with using a global section.   It is our preference to use this method because we need to support a legacy file format that needs to be parsed by other non-DPDK applications.  Those applications expect this format.  

Re: [PATCH 3/5] cfgfile: add support for unamed global section

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Friday, March 3, 2017 11:15 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jolliffe, Ian (Wind River) <ian.jolliffe@windriver.com>
> Subject: RE: [PATCH 3/5] cfgfile: add support for unamed global section
> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> >
> > I am not totally against it, but IMO this option is a bit confusing. Again,
> what's
> > wrong with user explicitly adding the GLOBAL section if needed?
> There's nothing wrong with using a global section.   It is our preference to use
> this method because we need to support a legacy file format that needs to
> be parsed by other non-DPDK applications.  Those applications expect this
> format.

OK, legacy is a good enough reason for me.

[PATCH 4/5] cfgfile: use strnlen to constrain memchr search

[Allain Legacy]

The call to memchr() uses the absolute length of the string buffer instead
of the actual length of the string returned by fgets().  This causes the
search to go beyond the '\n' character and find ';' characters in random
garbage on the stack.  This then causes the 'len' variable to be updated
and the subsequent search for the '=' character to potentially find one
beyond the first newline character.

Since this bug relies on ';' and '=' characters appearing in random places
in the 'buffer' variable it is intermittently reproducible at best.

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

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 2aba169..28956ea 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -133,7 +133,8 @@ struct rte_cfgfile *
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, RTE_LIBRTE_CFGFILE_COMMENT_CHAR, len);
+		pos = memchr(buffer, RTE_LIBRTE_CFGFILE_COMMENT_CHAR,
+			     sizeof(buffer));
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
-- 
1.8.3.1

[PATCH 5/5] cfgfile: increase local buffer size for max name and value

[Allain Legacy]

From: Joseph Richard <joseph.richard@windriver.com>

When parsing a ini file with a "key = value" line that has both "key" and
"value" sized to the maximum allowed length causes a parsing failure.  The
internal "buffer" variable should be sized at least as large as the maximum
for both fields.  This commit updates the local array to be sized to hold
the max name, max value, " = ", and the nul terminator.

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

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 28956ea..107d637 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -92,7 +92,7 @@ struct rte_cfgfile *
 	int allocated_entries = 0;
 	int curr_section = -1;
 	int curr_entry = -1;
-	char buffer[256] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
 	int lineno = 0;
 	size_t size;
 	struct rte_cfgfile *cfg = NULL;
-- 
1.8.3.1

Re: [PATCH 5/5] cfgfile: increase local buffer size for max name and value

[Wiles, Keith]

> On Mar 2, 2017, at 1:29 PM, Allain Legacy <allain.legacy@windriver.com> wrote:
> 
> From: Joseph Richard <joseph.richard@windriver.com>
> 
> When parsing a ini file with a "key = value" line that has both "key" and
> "value" sized to the maximum allowed length causes a parsing failure.  The
> internal "buffer" variable should be sized at least as large as the maximum
> for both fields.  This commit updates the local array to be sized to hold
> the max name, max value, " = ", and the nul terminator.
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
> lib/librte_cfgfile/rte_cfgfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 28956ea..107d637 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -92,7 +92,7 @@ struct rte_cfgfile *
> 	int allocated_entries = 0;
> 	int curr_section = -1;
> 	int curr_entry = -1;
> -	char buffer[256] = {0};
> +	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};

Would this change still cause a failure and memory over write if the user decides to have very large string. Does the code check the lengths to make sure they are valid and return error?

If the code is testing the size and make sure a memory over write does not happen, then I am OK with acking this patch. 

> 	int lineno = 0;
> 	size_t size;
> 	struct rte_cfgfile *cfg = NULL;
> -- 
> 1.8.3.1
> 

Regards,
Keith

Re: [PATCH 5/5] cfgfile: increase local buffer size for max name and value

[Legacy, Allain]

> -----Original Message-----
> From: Wiles, Keith [mailto:keith.wiles@intel.com]
> Sent: Thursday, March 09, 2017 8:46 AM
> Would this change still cause a failure and memory over write if the user
> decides to have very large string. Does the code check the lengths to make
> sure they are valid and return error?
> 

The fgets() is bounded by the size of the buffer and the subsequent validation will raise an error if no newline was detected within the buffer therefore an overly long line will result in a failure.  I have added a test case in the v2 patchset in which I have added a unit test framework for this library.

	while (fgets(buffer, sizeof(buffer), f) != NULL) {
		char *pos = NULL;
		size_t len = strnlen(buffer, sizeof(buffer));
		lineno++;
		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
			printf("Error line %d - no \\n found on string. "
					"Check if line too long\n", lineno);
			goto error1;
		}

Does that satisfy your concern and qualify for you Ack?

Re: [PATCH 5/5] cfgfile: increase local buffer size for max name and value

[Wiles, Keith]

> On Mar 9, 2017, at 9:16 AM, Legacy, Allain <Allain.Legacy@windriver.com> wrote:
> 
>> -----Original Message-----
>> From: Wiles, Keith [mailto:keith.wiles@intel.com]
>> Sent: Thursday, March 09, 2017 8:46 AM
>> Would this change still cause a failure and memory over write if the user
>> decides to have very large string. Does the code check the lengths to make
>> sure they are valid and return error?
>> 
> 
> The fgets() is bounded by the size of the buffer and the subsequent validation will raise an error if no newline was detected within the buffer therefore an overly long line will result in a failure.  I have added a test case in the v2 patchset in which I have added a unit test framework for this library.
> 
> 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
> 		char *pos = NULL;
> 		size_t len = strnlen(buffer, sizeof(buffer));
> 		lineno++;
> 		if ((len >= sizeof(buffer) - 1) && (buffer[len-1] != '\n')) {
> 			printf("Error line %d - no \\n found on string. "
> 					"Check if line too long\n", lineno);
> 			goto error1;
> 		}
> 
> Does that satisfy your concern and qualify for you Ack?

Acked-by: Keith Wiles <keith.wiles@intel.com>

Regards,
Keith

[PATCH v2 0/6] librte_cfgfile enhancements

[Allain Legacy]

This patchset includes some minor enhancements that we have developped for
our DPDK application.  We would like to contribute them upstream to help
ease adoption of the DPDK by anyone looking for this type of
functionality.  The commit logs on each patch should be self-sufficient in
explaining the intent and purpose.

v2:
* Added unit tests for the cfgfile library in the initial patch of the
  series and then added additional tests in subsequent patches where
  appropriate.  These will not run unless the following config parameter is
  set and additional packages are installed (e.g., libarchive-dev):
        CONFIG_RTE_APP_TEST_RESOURCE_TAR=y
* Reworked the configurable comment character patch to allow specifying a
  different character at runtime rather than build time.  Used a separate
  API to avoid affecting existing users or users that choose not to
  leverage the extended API.  Used a "parameters" structure to pass
  additional arguments rather than adding more arguments to the function to
  allow expansion in the future with minimal impact on existing users.
* Dropped the patch to initialize the cfg structure because the segfault
  that this was trying to address was already fixed by 2 earlier commits
  which we did not have in our development environment.  I realized this
  while trying to add unit tests to catch the segfault case.
* Fixed the doxygen comments related to the RTE_CFG_GLOBAL_SECTION patch
* Added an additional patch to allow parsing a key with an empty value
  (i.e., "key=").  I realized that I had forgotten to include this in my
  first patchset.

Allain Legacy (5):
  test: basic unit tests for cfgfile
  cfgfile: add support for unamed global section
  cfgfile: configurable comment character
  cfgfile: use strnlen to constrain memchr search
  cfgfile: add support for empty value string

Joseph Richard (1):
  cfgfile: increase local buffer size for max name and value

 config/common_base               |  1 +
 lib/librte_cfgfile/rte_cfgfile.c | 28 +++++++++++++++++++++++-----
 lib/librte_cfgfile/rte_cfgfile.h |  6 ++++++
 3 files changed, 30 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/6] test: basic unit tests for cfgfile

[Allain Legacy]

This commit adds the basic infrastructure for the cfgfile library unit
tests.  It includes success path tests for the most commonly used APIs.
More unit tests will be added later.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 MAINTAINERS                                       |   1 +
 test/test/Makefile                                |   2 +
 test/test/test_cfgfile.c                          | 210 ++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini             |   0
 test/test/test_cfgfiles/etc/invalid_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini   |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini     |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini   |   2 +
 test/test/test_cfgfiles/etc/sample1.ini           |  12 ++
 9 files changed, 236 insertions(+)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini

diff --git a/MAINTAINERS b/MAINTAINERS
index 5030c1c..16d5dd8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -598,6 +598,7 @@ Other libraries
 Configuration file
 M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
 F: lib/librte_cfgfile/
+F: test/test/test_cfgfile*
 
 Interactive command line
 M: Olivier Matz <olivier.matz@6wind.com>
diff --git a/test/test/Makefile b/test/test/Makefile
index 1a5e03d..622e795 100644
--- a/test/test/Makefile
+++ b/test/test/Makefile
@@ -85,6 +85,8 @@ $(eval $(call linked_resource,test_resource_c,test_resource.res))
 $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
 SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
 $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
+SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_cfgfile.c
+$(eval $(call linked_tar_resource,test_cfgfiles,test_cfgfiles))
 SRCS-y += test_prefetch.c
 SRCS-y += test_byteorder.c
 SRCS-y += test_per_lcore.c
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
new file mode 100644
index 0000000..fae2e0a
--- /dev/null
+++ b/test/test/test_cfgfile.c
@@ -0,0 +1,210 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Wind River Systems Inc. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_cfgfile.h>
+
+#include "test.h"
+#include "resource.h"
+
+
+#define CFG_FILES_ETC "test_cfgfiles/etc"
+
+REGISTER_LINKED_RESOURCE(test_cfgfiles);
+
+static int
+test_cfgfile_setup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_untar(r);
+	TEST_ASSERT_SUCCESS(ret, "failed to untar %s", r->name);
+
+	return 0;
+}
+
+static int
+test_cfgfile_cleanup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_rm_by_tar(r);
+	TEST_ASSERT_SUCCESS(ret, "Failed to delete resource %s", r->name);
+
+	return 0;
+}
+
+static int
+_test_cfgfile_sample(struct rte_cfgfile *cfgfile)
+{
+	const char *value;
+	int ret;
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 2, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key1");
+	TEST_ASSERT(strcmp("value1", value) == 0,
+		    "key1 unexpected value: %s", value);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section2");
+	TEST_ASSERT(ret, "section2 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section2");
+	TEST_ASSERT(ret == 2, "section2 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key2");
+	TEST_ASSERT(strcmp("value2", value) == 0,
+		    "key2 unexpected value: %s", value);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key3");
+	TEST_ASSERT(strcmp("value3", value) == 0,
+		    "key3 unexpected value: %s", value);
+
+	return 0;
+}
+
+static int
+test_cfgfile_sample1(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/sample1.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_section_header(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_key_value.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_missing_section(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_empty_file(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 0, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile(void)
+{
+	if (test_cfgfile_setup())
+		return -1;
+
+	if (test_cfgfile_sample1())
+		return -1;
+
+	if (test_cfgfile_invalid_section_header())
+		return -1;
+
+	if (test_cfgfile_invalid_key_value_pair())
+		return -1;
+
+	if (test_cfgfile_missing_section())
+		return -1;
+
+	if (test_cfgfile_empty_file())
+		return -1;
+
+	if (test_cfgfile_cleanup())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(cfgfile_autotest, test_cfgfile);
diff --git a/test/test/test_cfgfiles/etc/empty.ini b/test/test/test_cfgfiles/etc/empty.ini
new file mode 100644
index 0000000..e69de29
diff --git a/test/test/test_cfgfiles/etc/invalid_key_value.ini b/test/test/test_cfgfiles/etc/invalid_key_value.ini
new file mode 100644
index 0000000..8f203ce
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/invalid_key_value.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+invalid=
diff --git a/test/test/test_cfgfiles/etc/invalid_section.ini b/test/test/test_cfgfiles/etc/invalid_section.ini
new file mode 100644
index 0000000..95d6803
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/invalid_section.ini
@@ -0,0 +1,3 @@
+[invalid
+; this is section 1
+key1=value1
diff --git a/test/test/test_cfgfiles/etc/line_too_long.ini b/test/test/test_cfgfiles/etc/line_too_long.ini
new file mode 100644
index 0000000..1dce164
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/line_too_long.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
diff --git a/test/test/test_cfgfiles/etc/missing_section.ini b/test/test/test_cfgfiles/etc/missing_section.ini
new file mode 100644
index 0000000..c78e131
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/missing_section.ini
@@ -0,0 +1,2 @@
+; no section
+key=value
diff --git a/test/test/test_cfgfiles/etc/sample1.ini b/test/test/test_cfgfiles/etc/sample1.ini
new file mode 100644
index 0000000..aef91c2
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample1.ini
@@ -0,0 +1,12 @@
+; this is a global comment
+
+[section1]
+; this is section 1
+key1=value1
+
+[section2]
+; this is section 2
+;key1=value1
+key2=value2
+key3=value3 ; this is key3
+ignore-missing-separator
-- 
1.8.3.1

[PATCH v2 2/6] cfgfile: add support for unamed global section

[Allain Legacy]

The current implementation of the cfgfile library requires that all
key=value pairs be within [SECTION] definitions.  The ini file standard
allows for key=value pairs in an unnamed section.

   https://en.wikipedia.org/wiki/INI_file#Global_properties

This commit adds the capability of parsing key=value pairs from such an
unnamed section. The CFG_FLAG_GLOBAL_SECTION flag must be passed to the
rte_cfgfile_load() API to enable this functionality.  Any key=value pairs
found before the first section can be accessed in the section named
"GLOBAL".

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 16 ++++++++++++++++
 lib/librte_cfgfile/rte_cfgfile.h | 10 +++++++++-
 test/test/test_cfgfile.c         | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 829109a..832fea8 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -107,6 +107,22 @@ struct rte_cfgfile *
 
 	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
 
+	if (flags & CFG_FLAG_GLOBAL_SECTION) {
+		curr_section = 0;
+		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
+		cfg->sections[curr_section] = malloc(
+			sizeof(*cfg->sections[0]) +
+			sizeof(cfg->sections[0]->entries[0]) *
+			allocated_entries);
+		if (cfg->sections[curr_section] == NULL) {
+			printf("Error - no memory for global section\n");
+			goto error1;
+		}
+
+		snprintf(cfg->sections[curr_section]->name,
+				 sizeof(cfg->sections[0]->name), "GLOBAL");
+	}
+
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
 		size_t len = strnlen(buffer, sizeof(buffer));
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index b40e6a1..0e805c2 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,13 +66,21 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/**@{ cfgfile load operation flags */
+/**
+ * Indicates that the file supports key value entries before the first defined
+ * section.  These entries can be accessed in the "GLOBAL" section.
+ */
+#define CFG_FLAG_GLOBAL_SECTION (1 << 0)
+/**@} */
+
 /**
 * Open config file
 *
 * @param filename
 *   Config file name
 * @param flags
-*   Config file flags, Reserved for future use. Must be set to 0.
+*   Config file flags
 * @return
 *   Handle to configuration file on success, NULL otherwise
 */
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index fae2e0a..dd7afae 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -163,6 +163,36 @@
 }
 
 static int
+test_cfgfile_global_properties(void)
+{
+	struct rte_cfgfile *cfgfile;
+	const char *value;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini",
+				   CFG_FLAG_GLOBAL_SECTION);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Expected failured did not occur");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "GLOBAL");
+	TEST_ASSERT(ret, "global section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "GLOBAL");
+	TEST_ASSERT(ret == 1, "GLOBAL unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "GLOBAL", "key");
+	TEST_ASSERT(strcmp("value", value) == 0,
+		    "key unexpected value: %s", value);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_empty_file(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -198,6 +228,9 @@
 	if (test_cfgfile_missing_section())
 		return -1;
 
+	if (test_cfgfile_global_properties())
+		return -1;
+
 	if (test_cfgfile_empty_file())
 		return -1;
 
-- 
1.8.3.1

[PATCH v2 3/6] cfgfile: configurable comment character

[Allain Legacy]

The current cfgfile comment character is hardcoded to ';'.  This commit a
new API to allow the user to specify which comment character to use while
parsing the file.

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.

The approach of using a new API with an extensible parameters structure was
used rather than simply adding a new argument to the existing API to allow
for additional arguments to be introduced in the future.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c        | 26 ++++++++++++++++++++++---
 lib/librte_cfgfile/rte_cfgfile.h        | 34 +++++++++++++++++++++++++++++++++
 test/test/test_cfgfile.c                | 29 ++++++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/sample2.ini | 12 ++++++++++++
 4 files changed, 98 insertions(+), 3 deletions(-)
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 832fea8..7ecab22 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -85,9 +85,29 @@ struct rte_cfgfile {
 	return newlen;
 }
 
+void
+rte_cfgfile_init_parameters(struct rte_cfgfile_parameters *params)
+{
+	memset(params, 0, sizeof(*params));
+	params->comment_character = CFG_DEFAULT_COMMENT_CHARACTER;
+}
+
 struct rte_cfgfile *
 rte_cfgfile_load(const char *filename, int flags)
 {
+	struct rte_cfgfile_parameters params;
+
+	/* setup default parameter are add specified flags */
+	rte_cfgfile_init_parameters(&params);
+	params.flags |= flags;
+
+	return rte_cfgfile_load_with_params(filename, &params);
+}
+
+struct rte_cfgfile *
+rte_cfgfile_load_with_params(const char *filename,
+			     const struct rte_cfgfile_parameters *params)
+{
 	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
 	int allocated_entries = 0;
 	int curr_section = -1;
@@ -107,7 +127,7 @@ struct rte_cfgfile *
 
 	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
 
-	if (flags & CFG_FLAG_GLOBAL_SECTION) {
+	if (params->flags & CFG_FLAG_GLOBAL_SECTION) {
 		curr_section = 0;
 		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
 		cfg->sections[curr_section] = malloc(
@@ -132,7 +152,7 @@ struct rte_cfgfile *
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, ';', sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, sizeof(buffer));
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
@@ -242,7 +262,7 @@ struct rte_cfgfile *
 		}
 	}
 	fclose(f);
-	cfg->flags = flags;
+	cfg->flags = params->flags;
 	cfg->num_sections = curr_section + 1;
 	/* curr_section will still be -1 if we have an empty file */
 	if (curr_section >= 0)
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 0e805c2..069bbd4 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/** Configuration file operation optional arguments */
+struct rte_cfgfile_parameters {
+	int flags; /**< Config file flags */
+	char comment_character; /**< Config file comment character */
+};
+
 /**@{ cfgfile load operation flags */
 /**
  * Indicates that the file supports key value entries before the first defined
@@ -74,6 +80,17 @@ struct rte_cfgfile_entry {
 #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
 /**@} */
 
+/** Defines the default comment character used for parsing config files. */
+#define CFG_DEFAULT_COMMENT_CHARACTER ';'
+
+/**
+ * Initialize config file optional parameters to default values.
+ *
+ * @param params
+ *   parameters to be initialized
+ */
+void rte_cfgfile_init_parameters(struct rte_cfgfile_parameters *params);
+
 /**
 * Open config file
 *
@@ -87,6 +104,23 @@ struct rte_cfgfile_entry {
 struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
 
 /**
+ * Open config file with specified optional parameters.  Use @see
+ * rte_cfgfile_init_parameters to setup the default parameters.
+ *
+ * @param filename
+ *   Config file name
+ * @param params
+ *   Config file flags
+ * @return
+ *   Handle to configuration file on success, NULL otherwise
+ * @param
+ *
+ */
+struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
+	const struct rte_cfgfile_parameters *params);
+
+
+/**
 * Get number of sections in config file
 *
 * @param cfg
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index dd7afae..eab8ccc 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -130,6 +130,32 @@
 }
 
 static int
+test_cfgfile_sample2(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	/* setup default */
+	rte_cfgfile_init_parameters(&params);
+
+	/* override comment character */
+	params.comment_character = '#';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini",
+					       &params);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_section_header(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -219,6 +245,9 @@
 	if (test_cfgfile_sample1())
 		return -1;
 
+	if (test_cfgfile_sample2())
+		return -1;
+
 	if (test_cfgfile_invalid_section_header())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/sample2.ini b/test/test/test_cfgfiles/etc/sample2.ini
new file mode 100644
index 0000000..21075e9
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample2.ini
@@ -0,0 +1,12 @@
+# this is a global comment
+
+[section1]
+# this is section 1
+key1=value1
+
+[section2]
+# this is section 2
+#key1=value1
+key2=value2
+key3=value3 # this is key3
+ignore-missing-separator
-- 
1.8.3.1

Re: [PATCH v2 3/6] cfgfile: configurable comment character

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Thursday, March 9, 2017 1:11 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> Subject: [PATCH v2 3/6] cfgfile: configurable comment character
> 
> The current cfgfile comment character is hardcoded to ';'.  This commit a
> new API to allow the user to specify which comment character to use while
> parsing the file.
> 
> 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.
> 
> The approach of using a new API with an extensible parameters structure
> was
> used rather than simply adding a new argument to the existing API to allow
> for additional arguments to be introduced in the future.
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c        | 26 ++++++++++++++++++++++---
>  lib/librte_cfgfile/rte_cfgfile.h        | 34
> +++++++++++++++++++++++++++++++++
>  test/test/test_cfgfile.c                | 29 ++++++++++++++++++++++++++++
>  test/test/test_cfgfiles/etc/sample2.ini | 12 ++++++++++++
>  4 files changed, 98 insertions(+), 3 deletions(-)
>  create mode 100644 test/test/test_cfgfiles/etc/sample2.ini
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 832fea8..7ecab22 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -85,9 +85,29 @@ struct rte_cfgfile {
>  	return newlen;
>  }
> 
> +void
> +rte_cfgfile_init_parameters(struct rte_cfgfile_parameters *params)
> +{
> +	memset(params, 0, sizeof(*params));
> +	params->comment_character =
> CFG_DEFAULT_COMMENT_CHARACTER;
> +}
> +

I don't think we need this API function, as it brings no value to the user.

We can simply define the structure with the default values as static variable in this file and pass it directly as argument to load_with_params() when called by the load() function.

>  struct rte_cfgfile *
>  rte_cfgfile_load(const char *filename, int flags)
>  {
> +	struct rte_cfgfile_parameters params;
> +
> +	/* setup default parameter are add specified flags */
> +	rte_cfgfile_init_parameters(&params);
> +	params.flags |= flags;
> +
> +	return rte_cfgfile_load_with_params(filename, &params);
> +}
> +
> +struct rte_cfgfile *
> +rte_cfgfile_load_with_params(const char *filename,
> +			     const struct rte_cfgfile_parameters *params)

I like this approach, but please keep the flags parameter in the signature of the new API function load_with_params(). I am OK with the params structure having just single field for now.

> +{
>  	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
>  	int allocated_entries = 0;
>  	int curr_section = -1;
> @@ -107,7 +127,7 @@ struct rte_cfgfile *
> 
>  	memset(cfg->sections, 0, sizeof(cfg->sections[0]) *
> allocated_sections);
> 
> -	if (flags & CFG_FLAG_GLOBAL_SECTION) {
> +	if (params->flags & CFG_FLAG_GLOBAL_SECTION) {
>  		curr_section = 0;
>  		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
>  		cfg->sections[curr_section] = malloc(
> @@ -132,7 +152,7 @@ struct rte_cfgfile *
>  					"Check if line too long\n", lineno);
>  			goto error1;
>  		}
> -		pos = memchr(buffer, ';', sizeof(buffer));
> +		pos = memchr(buffer, params->comment_character,
> sizeof(buffer));
>  		if (pos != NULL) {
>  			*pos = '\0';
>  			len = pos -  buffer;
> @@ -242,7 +262,7 @@ struct rte_cfgfile *
>  		}
>  	}

Per previous feedback, please check the value of the comment_character against invalid options.

I suggest creating a new static function in the file for validating the parameter structure, e.g.
	int rte_cfgfile_check_params(struct rte_cfgfile_parameters *params)

Copy & paste from previous feedback (http://www.dpdk.org/ml/archives/dev/2017-March/059159.html):
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.

>  	fclose(f);
> -	cfg->flags = flags;
> +	cfg->flags = params->flags;
>  	cfg->num_sections = curr_section + 1;
>  	/* curr_section will still be -1 if we have an empty file */
>  	if (curr_section >= 0)
> diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
> index 0e805c2..069bbd4 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.h
> +++ b/lib/librte_cfgfile/rte_cfgfile.h
> @@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
>  	char value[CFG_VALUE_LEN]; /**< Value */
>  };
> 
> +/** Configuration file operation optional arguments */
> +struct rte_cfgfile_parameters {
> +	int flags; /**< Config file flags */

Per above comment, the flags parameter should be kept in the prototype of the new load_with_params() API function as opposed to being moved here.

> +	char comment_character; /**< Config file comment character */
> +};
> +
>  /**@{ cfgfile load operation flags */
>  /**
>   * Indicates that the file supports key value entries before the first defined
> @@ -74,6 +80,17 @@ struct rte_cfgfile_entry {
>  #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
>  /**@} */
> 
> +/** Defines the default comment character used for parsing config files. */
> +#define CFG_DEFAULT_COMMENT_CHARACTER ';'
> +
> +/**
> + * Initialize config file optional parameters to default values.
> + *
> + * @param params
> + *   parameters to be initialized
> + */
> +void rte_cfgfile_init_parameters(struct rte_cfgfile_parameters *params);
> +
>  /**
>  * Open config file
>  *
> @@ -87,6 +104,23 @@ struct rte_cfgfile_entry {
>  struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
> 
>  /**
> + * Open config file with specified optional parameters.  Use @see
> + * rte_cfgfile_init_parameters to setup the default parameters.
> + *
> + * @param filename
> + *   Config file name
> + * @param params
> + *   Config file flags
> + * @return
> + *   Handle to configuration file on success, NULL otherwise
> + * @param
> + *
> + */
> +struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
> +	const struct rte_cfgfile_parameters *params);
> +
> +
> +/**
>  * Get number of sections in config file
>  *
>  * @param cfg
> diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
> index dd7afae..eab8ccc 100644
> --- a/test/test/test_cfgfile.c
> +++ b/test/test/test_cfgfile.c
> @@ -130,6 +130,32 @@
>  }
> 
>  static int
> +test_cfgfile_sample2(void)
> +{
> +	struct rte_cfgfile_parameters params;
> +	struct rte_cfgfile *cfgfile;
> +	int ret;
> +
> +	/* setup default */
> +	rte_cfgfile_init_parameters(&params);
> +
> +	/* override comment character */
> +	params.comment_character = '#';
> +
> +	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC
> "/sample2.ini",
> +					       &params);
> +	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
> +
> +	ret = _test_cfgfile_sample(cfgfile);
> +	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
> +
> +	ret = rte_cfgfile_close(cfgfile);
> +	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
> +
> +	return 0;
> +}
> +
> +static int
>  test_cfgfile_invalid_section_header(void)
>  {
>  	struct rte_cfgfile *cfgfile;
> @@ -219,6 +245,9 @@
>  	if (test_cfgfile_sample1())
>  		return -1;
> 
> +	if (test_cfgfile_sample2())
> +		return -1;
> +
>  	if (test_cfgfile_invalid_section_header())
>  		return -1;
> 
> diff --git a/test/test/test_cfgfiles/etc/sample2.ini
> b/test/test/test_cfgfiles/etc/sample2.ini
> new file mode 100644
> index 0000000..21075e9
> --- /dev/null
> +++ b/test/test/test_cfgfiles/etc/sample2.ini
> @@ -0,0 +1,12 @@
> +# this is a global comment
> +
> +[section1]
> +# this is section 1
> +key1=value1
> +
> +[section2]
> +# this is section 2
> +#key1=value1
> +key2=value2
> +key3=value3 # this is key3
> +ignore-missing-separator
> --
> 1.8.3.1

We do not want to allow in DPDK config files that use comment characters that are not allowed by any other commonly used file parser. Allowing invalid options is likely to add confusion for the user and a bad perception over DPDK.

This was already provided as feedback and dropped for no reason. Until this get implemented,

NAK

[PATCH v2 4/6] cfgfile: use strnlen to constrain memchr search

[Allain Legacy]

The call to memchr() uses the absolute length of the string buffer instead
of the actual length of the string returned by fgets().  This causes the
search to go beyond the '\n' character and find ';' characters in random
garbage on the stack.  This then causes the 'len' variable to be updated
and the subsequent search for the '=' character to potentially find one
beyond the first newline character.

Since this bug relies on ';' and '=' characters appearing in random places
in the 'buffer' variable it is intermittently reproducible at best.

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

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 7ecab22..416b3b6 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -152,7 +152,7 @@ struct rte_cfgfile *
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, params->comment_character, sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, len);
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
-- 
1.8.3.1

[PATCH v2 5/6] cfgfile: increase local buffer size for max name and value

[Allain Legacy]

From: Joseph Richard <joseph.richard@windriver.com>

When parsing a ini file with a "key = value" line that has both "key" and
"value" sized to the maximum allowed length causes a parsing failure.  The
internal "buffer" variable should be sized at least as large as the maximum
for both fields.  This commit updates the local array to be sized to hold
the max name, max value, " = ", and the nul terminator.

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

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 416b3b6..35c81e4 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -112,7 +112,7 @@ struct rte_cfgfile *
 	int allocated_entries = 0;
 	int curr_section = -1;
 	int curr_entry = -1;
-	char buffer[256] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
-- 
1.8.3.1

[PATCH v2 6/6] cfgfile: add support for empty value string

[Allain Legacy]

This commit adds support to the cfgfile library for parsing a key=value
line that has no value string specified (e.g., "key=").  This can be used
to override a configuration attribute that has a default value or default
list of values to set it back to an undefined value to disable
functionality.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c        | 11 ++++++-----
 test/test/test_cfgfile.c                | 17 +----------------
 test/test/test_cfgfiles/etc/sample1.ini |  2 +-
 test/test/test_cfgfiles/etc/sample2.ini |  2 +-
 4 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 35c81e4..d3022b9 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -219,11 +219,12 @@ struct rte_cfgfile *
 
 			struct rte_cfgfile_section *sect =
 				cfg->sections[curr_section];
-			char *split[2];
-			if (rte_strsplit(buffer, sizeof(buffer), split, 2, '=')
-				!= 2) {
+			int n;
+			char *split[2] = {NULL};
+			n = rte_strsplit(buffer, sizeof(buffer), split, 2, '=');
+			if ((n < 1) || (n > 2)) {
 				printf("Error at line %d - cannot split "
-					"string\n", lineno);
+				       "string, n=%d\n", lineno, n);
 				goto error1;
 			}
 
@@ -254,7 +255,7 @@ struct rte_cfgfile *
 			snprintf(entry->name, sizeof(entry->name), "%s",
 				split[0]);
 			snprintf(entry->value, sizeof(entry->value), "%s",
-				split[1]);
+				 split[1] ? split[1] : "");
 			_strip(entry->name, strnlen(entry->name,
 				sizeof(entry->name)));
 			_strip(entry->value, strnlen(entry->value,
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index eab8ccc..d8214d1 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -105,8 +105,7 @@
 		    "key2 unexpected value: %s", value);
 
 	value = rte_cfgfile_get_entry(cfgfile, "section2", "key3");
-	TEST_ASSERT(strcmp("value3", value) == 0,
-		    "key3 unexpected value: %s", value);
+	TEST_ASSERT(strlen(value) == 0, "key3 unexpected value: %s", value);
 
 	return 0;
 }
@@ -167,17 +166,6 @@
 }
 
 static int
-test_cfgfile_invalid_key_value_pair(void)
-{
-	struct rte_cfgfile *cfgfile;
-
-	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_key_value.ini", 0);
-	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
-
-	return 0;
-}
-
-static int
 test_cfgfile_missing_section(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -251,9 +239,6 @@
 	if (test_cfgfile_invalid_section_header())
 		return -1;
 
-	if (test_cfgfile_invalid_key_value_pair())
-		return -1;
-
 	if (test_cfgfile_missing_section())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/sample1.ini b/test/test/test_cfgfiles/etc/sample1.ini
index aef91c2..5b001d3 100644
--- a/test/test/test_cfgfiles/etc/sample1.ini
+++ b/test/test/test_cfgfiles/etc/sample1.ini
@@ -8,5 +8,5 @@ key1=value1
 ; this is section 2
 ;key1=value1
 key2=value2
-key3=value3 ; this is key3
+key3= ; this is key3
 ignore-missing-separator
diff --git a/test/test/test_cfgfiles/etc/sample2.ini b/test/test/test_cfgfiles/etc/sample2.ini
index 21075e9..8fee5a7 100644
--- a/test/test/test_cfgfiles/etc/sample2.ini
+++ b/test/test/test_cfgfiles/etc/sample2.ini
@@ -8,5 +8,5 @@ key1=value1
 # this is section 2
 #key1=value1
 key2=value2
-key3=value3 # this is key3
+key3= # this is key3
 ignore-missing-separator
-- 
1.8.3.1

Re: [PATCH v2 6/6] cfgfile: add support for empty value string

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Thursday, March 9, 2017 1:11 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>
> Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> Subject: [PATCH v2 6/6] cfgfile: add support for empty value string
> 
> This commit adds support to the cfgfile library for parsing a key=value
> line that has no value string specified (e.g., "key=").  This can be used
> to override a configuration attribute that has a default value or default
> list of values to set it back to an undefined value to disable
> functionality.
> 

IMO allowing empty string key values is confusing and should not be allowed.

I think there are better alternatives for setting a key to its default value:
	key = default
	key = DEFAULT
	key = <the specific default value>

Any reason not to use these approaches?

Re: [PATCH v2 6/6] cfgfile: add support for empty value string

[Legacy, Allain]

We have a legacy file format that we need to support.   Other parts of our system are able to handle a "key=" entry in the file so we are trying to gain parity with those parsers. 

Allain


Allain Legacy, Software Developer
direct 613.270.2279  fax 613.492.7870 skype allain.legacy
 



> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> Sent: Monday, March 27, 2017 6:55 AM
> To: Legacy, Allain; RICHARDSON, BRUCE
> Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> Subject: RE: [PATCH v2 6/6] cfgfile: add support for empty value string
> 
> 
> 
> > -----Original Message-----
> > From: Allain Legacy [mailto:allain.legacy@windriver.com]
> > Sent: Thursday, March 9, 2017 1:11 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>; Dumitrescu,
> > Cristian <cristian.dumitrescu@intel.com>
> > Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> > Subject: [PATCH v2 6/6] cfgfile: add support for empty value string
> >
> > This commit adds support to the cfgfile library for parsing a
> > key=value line that has no value string specified (e.g., "key=").
> > This can be used to override a configuration attribute that has a
> > default value or default list of values to set it back to an undefined
> > value to disable functionality.
> >
> 
> IMO allowing empty string key values is confusing and should not be allowed.
> 
> I think there are better alternatives for setting a key to its default value:
> 	key = default
> 	key = DEFAULT
> 	key = <the specific default value>
> 
> Any reason not to use these approaches?

Re: [PATCH v2 6/6] cfgfile: add support for empty value string

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Legacy, Allain [mailto:Allain.Legacy@windriver.com]
> Sent: Monday, March 27, 2017 12:13 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> Subject: RE: [PATCH v2 6/6] cfgfile: add support for empty value string
> 
> We have a legacy file format that we need to support.   Other parts of our
> system are able to handle a "key=" entry in the file so we are trying to gain
> parity with those parsers.
> 
> Allain
> 
> 
> Allain Legacy, Software Developer
> direct 613.270.2279  fax 613.492.7870 skype allain.legacy
> 
> 
> 

OK, for legacy reasons I am OK to allow this feature, but please let's have the user explicitly ask for it by providing a specific flag for it in the flags argument of the load/load_with_params functions.

> 
> > -----Original Message-----
> > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> > Sent: Monday, March 27, 2017 6:55 AM
> > To: Legacy, Allain; RICHARDSON, BRUCE
> > Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> > Subject: RE: [PATCH v2 6/6] cfgfile: add support for empty value string
> >
> >
> >
> > > -----Original Message-----
> > > From: Allain Legacy [mailto:allain.legacy@windriver.com]
> > > Sent: Thursday, March 9, 2017 1:11 PM
> > > To: Richardson, Bruce <bruce.richardson@intel.com>; Dumitrescu,
> > > Cristian <cristian.dumitrescu@intel.com>
> > > Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> > > Subject: [PATCH v2 6/6] cfgfile: add support for empty value string
> > >
> > > This commit adds support to the cfgfile library for parsing a
> > > key=value line that has no value string specified (e.g., "key=").
> > > This can be used to override a configuration attribute that has a
> > > default value or default list of values to set it back to an undefined
> > > value to disable functionality.
> > >
> >
> > IMO allowing empty string key values is confusing and should not be
> allowed.
> >
> > I think there are better alternatives for setting a key to its default value:
> > 	key = default
> > 	key = DEFAULT
> > 	key = <the specific default value>
> >
> > Any reason not to use these approaches?
> 

Re: [PATCH v2 6/6] cfgfile: add support for empty value string

[Legacy, Allain]

> -----Original Message-----
> From: Legacy, Allain
> Sent: Monday, March 27, 2017 7:13 AM
> To: DUMITRESCU, CRISTIAN FLORIN; RICHARDSON, BRUCE
> Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org
> Subject: RE: [PATCH v2 6/6] cfgfile: add support for empty value string
> 
> We have a legacy file format that we need to support.   Other parts of our
> system are able to handle a "key=" entry in the file so we are trying to gain
> parity with those parsers.
> 
I should have also mentioned that I briefly considered changing rte_strsplit() to be able to handle that case because in python the split function handles this scenario in the desired way:

	a = "key="
	a.split('=')
	['key', '']

But, I figured that change would be too intrusive and would likely be rejected.

Allain

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Thomas Monjalon]

2017-03-09 08:10, Allain Legacy:
> This patchset includes some minor enhancements that we have developped for
> our DPDK application.  We would like to contribute them upstream to help
> ease adoption of the DPDK by anyone looking for this type of
> functionality.  The commit logs on each patch should be self-sufficient in
> explaining the intent and purpose.

This series is small enough to be merged.
However, in the long term, we should not have this kind of library in DPDK.

librte_cfgfile is used by the examples ip_pipeline and qos_sched.
I think the purpose of an example is to show some simple code demonstrating
a feature.
Examples using a configuration file are closer to a complete application.

Anyway, why not use an external library like this one?
	https://github.com/vstakhov/libucl

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Bruce Richardson]

On Tue, Mar 28, 2017 at 10:29:44AM +0200, Thomas Monjalon wrote:
> 2017-03-09 08:10, Allain Legacy:
> > This patchset includes some minor enhancements that we have developped for
> > our DPDK application.  We would like to contribute them upstream to help
> > ease adoption of the DPDK by anyone looking for this type of
> > functionality.  The commit logs on each patch should be self-sufficient in
> > explaining the intent and purpose.
> 
> This series is small enough to be merged.
> However, in the long term, we should not have this kind of library in DPDK.
> 
> librte_cfgfile is used by the examples ip_pipeline and qos_sched.
> I think the purpose of an example is to show some simple code demonstrating
> a feature.
> Examples using a configuration file are closer to a complete application.
> 
> Anyway, why not use an external library like this one?
> 	https://github.com/vstakhov/libucl

Because as a general rule, anything adding in external dependencies needs
to be disabled by default. This leads to the catch-22 situation I
flagged before, and had no follow-up on. There is no way right now for
someone to put in extra functionality like this into DPDK and  have it
default enabled. If you try putting it into DPDK directly, it will be
rejected as duplicating other libs, but if you re-use the libs, it will
be disabled by default as adding in an extra dependency.

/Bruce

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Bruce Richardson]

On Tue, Mar 28, 2017 at 10:18:28AM +0100, Bruce Richardson wrote:
> On Tue, Mar 28, 2017 at 10:29:44AM +0200, Thomas Monjalon wrote:
> > 2017-03-09 08:10, Allain Legacy:
> > > This patchset includes some minor enhancements that we have developped for
> > > our DPDK application.  We would like to contribute them upstream to help
> > > ease adoption of the DPDK by anyone looking for this type of
> > > functionality.  The commit logs on each patch should be self-sufficient in
> > > explaining the intent and purpose.
> > 
> > This series is small enough to be merged.
> > However, in the long term, we should not have this kind of library in DPDK.
> > 
> > librte_cfgfile is used by the examples ip_pipeline and qos_sched.
> > I think the purpose of an example is to show some simple code demonstrating
> > a feature.
> > Examples using a configuration file are closer to a complete application.
> > 
> > Anyway, why not use an external library like this one?
> > 	https://github.com/vstakhov/libucl
> 
> Because as a general rule, anything adding in external dependencies needs
> to be disabled by default. This leads to the catch-22 situation I
> flagged before, and had no follow-up on. There is no way right now for
> someone to put in extra functionality like this into DPDK and  have it
> default enabled. If you try putting it into DPDK directly, it will be
> rejected as duplicating other libs, but if you re-use the libs, it will
> be disabled by default as adding in an extra dependency.
> 
> /Bruce

As follow-up to my own mail, for this specific library example, I
wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
trivial, so I think it's not a big deal to keep our own version and not
have an external dependency - especially since it's already there and not
a big maintenance burden.

For newer functionalty, we do need clear guidelines as to when it is
acceptable to add new dependencies to DPDK. I'd love to see us enable
the PCAP PMD by default, for instance, and I think Sergio has recently
proposed we also require libnuma on Linux.

Thoughts?

/Bruce

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Thomas Monjalon]

2017-03-28 10:22, Bruce Richardson:
> On Tue, Mar 28, 2017 at 10:18:28AM +0100, Bruce Richardson wrote:
> > On Tue, Mar 28, 2017 at 10:29:44AM +0200, Thomas Monjalon wrote:
> > > 2017-03-09 08:10, Allain Legacy:
> > > > This patchset includes some minor enhancements that we have developped for
> > > > our DPDK application.  We would like to contribute them upstream to help
> > > > ease adoption of the DPDK by anyone looking for this type of
> > > > functionality.  The commit logs on each patch should be self-sufficient in
> > > > explaining the intent and purpose.
> > > 
> > > This series is small enough to be merged.
> > > However, in the long term, we should not have this kind of library in DPDK.
> > > 
> > > librte_cfgfile is used by the examples ip_pipeline and qos_sched.
> > > I think the purpose of an example is to show some simple code demonstrating
> > > a feature.
> > > Examples using a configuration file are closer to a complete application.
> > > 
> > > Anyway, why not use an external library like this one?
> > > 	https://github.com/vstakhov/libucl
> > 
> > Because as a general rule, anything adding in external dependencies needs
> > to be disabled by default. This leads to the catch-22 situation I
> > flagged before, and had no follow-up on. There is no way right now for
> > someone to put in extra functionality like this into DPDK and  have it
> > default enabled. If you try putting it into DPDK directly, it will be
> > rejected as duplicating other libs, but if you re-use the libs, it will
> > be disabled by default as adding in an extra dependency.
> > 
> > /Bruce
> 
> As follow-up to my own mail, for this specific library example, I
> wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
> trivial, so I think it's not a big deal to keep our own version and not
> have an external dependency - especially since it's already there and not
> a big maintenance burden.

Removing this lib would not disable anything as it is used only by examples.
I don't see what would be the issue.
We just have to download the lib when building the example app.
It can be done quite easily in the makefile.

> For newer functionalty, we do need clear guidelines as to when it is
> acceptable to add new dependencies to DPDK. I'd love to see us enable
> the PCAP PMD by default, for instance, and I think Sergio has recently
> proposed we also require libnuma on Linux.

We won't include libpcap or libnuma.
The only thing we should do is to make easier to view and enable dependencies.

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Dumitrescu, Cristian]

> > As follow-up to my own mail, for this specific library example, I
> > wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
> > trivial, so I think it's not a big deal to keep our own version and not
> > have an external dependency - especially since it's already there and not
> > a big maintenance burden.
> 
> Removing this lib would not disable anything as it is used only by examples.
> I don't see what would be the issue.
> We just have to download the lib when building the example app.
> It can be done quite easily in the makefile.
> 

Thomas, more than 3 quarters of DPDK libs are only used by applications, is this a reason to remove them?

Also, I think the purpose of DPDK is to enable people to write applications, not more libraries. Would you agree? We should make the life easier for the application developers, not libraries.

This library is an important utility for applications, similar to librte_cmdline and others. I think it is not fair from your side to refer to librte_cfgfile without any reference to librte_cmdline.


> > For newer functionalty, we do need clear guidelines as to when it is
> > acceptable to add new dependencies to DPDK. I'd love to see us enable
> > the PCAP PMD by default, for instance, and I think Sergio has recently
> > proposed we also require libnuma on Linux.
> 
> We won't include libpcap or libnuma.
> The only thing we should do is to make easier to view and enable
> dependencies.

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Thomas Monjalon]

2017-03-28 09:58, Dumitrescu, Cristian:
> > > As follow-up to my own mail, for this specific library example, I
> > > wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
> > > trivial, so I think it's not a big deal to keep our own version and not
> > > have an external dependency - especially since it's already there and not
> > > a big maintenance burden.
> > 
> > Removing this lib would not disable anything as it is used only by examples.
> > I don't see what would be the issue.
> > We just have to download the lib when building the example app.
> > It can be done quite easily in the makefile.
> 
> Thomas, more than 3 quarters of DPDK libs are only used by applications, is this a reason to remove them?
> 
> Also, I think the purpose of DPDK is to enable people to write applications, not more libraries. Would you agree? We should make the life easier for the application developers, not libraries.
> 
> This library is an important utility for applications, similar to librte_cmdline and others. I think it is not fair from your side to refer to librte_cfgfile without any reference to librte_cmdline.

I agree Cristian.
I was just writing another email about removing librte_cmdline:
http://dpdk.org/ml/archives/dev/2017-March/061777.html
This thread was about librte_cfgfile. I hope you'll agree I am really fair :)

It is really a scope question and should be managed by the techboard (CC).

> > > For newer functionalty, we do need clear guidelines as to when it is
> > > acceptable to add new dependencies to DPDK. I'd love to see us enable
> > > the PCAP PMD by default, for instance, and I think Sergio has recently
> > > proposed we also require libnuma on Linux.
> > 
> > We won't include libpcap or libnuma.
> > The only thing we should do is to make easier to view and enable
> > dependencies.

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Dumitrescu, Cristian]

> > This library is an important utility for applications, similar to librte_cmdline
> and others. I think it is not fair from your side to refer to librte_cfgfile without
> any reference to librte_cmdline.
> 
> I agree Cristian.
> I was just writing another email about removing librte_cmdline:
> http://dpdk.org/ml/archives/dev/2017-March/061777.html
> This thread was about librte_cfgfile. I hope you'll agree I am really fair :)
> 
> It is really a scope question and should be managed by the techboard (CC).
> 

It is virtually impossible to write a non-trivial packet processing application without support for configuration file (librte_cfgfile) and CLI (librte_cmdline), therefore I think both of these building blocks are definitely within the scope of DPDK. As its name suggests, DPDK is a *development kit* for data plane applications, right?

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Bruce Richardson]

On Tue, Mar 28, 2017 at 12:12:26PM +0200, Thomas Monjalon wrote:
> 2017-03-28 09:58, Dumitrescu, Cristian:
> > > > As follow-up to my own mail, for this specific library example, I
> > > > wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
> > > > trivial, so I think it's not a big deal to keep our own version and not
> > > > have an external dependency - especially since it's already there and not
> > > > a big maintenance burden.
> > > 
> > > Removing this lib would not disable anything as it is used only by examples.
> > > I don't see what would be the issue.
> > > We just have to download the lib when building the example app.
> > > It can be done quite easily in the makefile.
> > 
> > Thomas, more than 3 quarters of DPDK libs are only used by applications, is this a reason to remove them?
> > 
> > Also, I think the purpose of DPDK is to enable people to write applications, not more libraries. Would you agree? We should make the life easier for the application developers, not libraries.
> > 
> > This library is an important utility for applications, similar to librte_cmdline and others. I think it is not fair from your side to refer to librte_cfgfile without any reference to librte_cmdline.
> 
> I agree Cristian.
> I was just writing another email about removing librte_cmdline:
> http://dpdk.org/ml/archives/dev/2017-March/061777.html
> This thread was about librte_cfgfile. I hope you'll agree I am really fair :)
> 
> It is really a scope question and should be managed by the techboard (CC).
> 
Sure.

As for my 2c right now on this lib, I'm very much in favour of keeping
it. I also think we should look to reuse it as an alternative way of
passing parameters to EAL. The existing method of using argc/argv makes
passing a lot of args, e.g for devices clunky, and re-using code from
cfgfile library gives us an alternative without adding extra
dependencies. I also think it could be useful for testpmd, which is
similarly "blessed" with lots of cmdline args to pass.

In short, I'd like to see this used more, rather than dropped. I think
we need a common config lib in DPDK. If not this, we need an external
mandatory dependency, and I'd prefer to stick with what we have for now.
At minimum, I think any removal of this is premature.

Regards,
/Bruce

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Thomas Monjalon]

2017-03-28 16:24, Bruce Richardson:
> On Tue, Mar 28, 2017 at 12:12:26PM +0200, Thomas Monjalon wrote:
> > 2017-03-28 09:58, Dumitrescu, Cristian:
> > > > > As follow-up to my own mail, for this specific library example, I
> > > > > wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
> > > > > trivial, so I think it's not a big deal to keep our own version and not
> > > > > have an external dependency - especially since it's already there and not
> > > > > a big maintenance burden.
> > > > 
> > > > Removing this lib would not disable anything as it is used only by examples.
> > > > I don't see what would be the issue.
> > > > We just have to download the lib when building the example app.
> > > > It can be done quite easily in the makefile.
> > > 
> > > Thomas, more than 3 quarters of DPDK libs are only used by applications, is this a reason to remove them?
> > > 
> > > Also, I think the purpose of DPDK is to enable people to write applications, not more libraries. Would you agree? We should make the life easier for the application developers, not libraries.
> > > 
> > > This library is an important utility for applications, similar to librte_cmdline and others. I think it is not fair from your side to refer to librte_cfgfile without any reference to librte_cmdline.
> > 
> > I agree Cristian.
> > I was just writing another email about removing librte_cmdline:
> > http://dpdk.org/ml/archives/dev/2017-March/061777.html
> > This thread was about librte_cfgfile. I hope you'll agree I am really fair :)
> > 
> > It is really a scope question and should be managed by the techboard (CC).
> > 
> Sure.
> 
> As for my 2c right now on this lib, I'm very much in favour of keeping
> it. I also think we should look to reuse it as an alternative way of
> passing parameters to EAL. The existing method of using argc/argv makes
> passing a lot of args, e.g for devices clunky, and re-using code from
> cfgfile library gives us an alternative without adding extra
> dependencies. I also think it could be useful for testpmd, which is
> similarly "blessed" with lots of cmdline args to pass.

I think we must improve the API and later deprecate argc/argv.
The configuration must be done by the application.
It should be the application choice to get its input from the command line,
a CLI or a config file.

Re: [PATCH v2 0/6] librte_cfgfile enhancements

[Bruce Richardson]

On Tue, Mar 28, 2017 at 05:41:01PM +0200, Thomas Monjalon wrote:
> 2017-03-28 16:24, Bruce Richardson:
> > On Tue, Mar 28, 2017 at 12:12:26PM +0200, Thomas Monjalon wrote:
> > > 2017-03-28 09:58, Dumitrescu, Cristian:
> > > > > > As follow-up to my own mail, for this specific library example, I
> > > > > > wouldn't look to remove it from DPDK anyway. Parsing ini files is fairly
> > > > > > trivial, so I think it's not a big deal to keep our own version and not
> > > > > > have an external dependency - especially since it's already there and not
> > > > > > a big maintenance burden.
> > > > > 
> > > > > Removing this lib would not disable anything as it is used only by examples.
> > > > > I don't see what would be the issue.
> > > > > We just have to download the lib when building the example app.
> > > > > It can be done quite easily in the makefile.
> > > > 
> > > > Thomas, more than 3 quarters of DPDK libs are only used by applications, is this a reason to remove them?
> > > > 
> > > > Also, I think the purpose of DPDK is to enable people to write applications, not more libraries. Would you agree? We should make the life easier for the application developers, not libraries.
> > > > 
> > > > This library is an important utility for applications, similar to librte_cmdline and others. I think it is not fair from your side to refer to librte_cfgfile without any reference to librte_cmdline.
> > > 
> > > I agree Cristian.
> > > I was just writing another email about removing librte_cmdline:
> > > http://dpdk.org/ml/archives/dev/2017-March/061777.html
> > > This thread was about librte_cfgfile. I hope you'll agree I am really fair :)
> > > 
> > > It is really a scope question and should be managed by the techboard (CC).
> > > 
> > Sure.
> > 
> > As for my 2c right now on this lib, I'm very much in favour of keeping
> > it. I also think we should look to reuse it as an alternative way of
> > passing parameters to EAL. The existing method of using argc/argv makes
> > passing a lot of args, e.g for devices clunky, and re-using code from
> > cfgfile library gives us an alternative without adding extra
> > dependencies. I also think it could be useful for testpmd, which is
> > similarly "blessed" with lots of cmdline args to pass.
> 
> I think we must improve the API and later deprecate argc/argv.
> The configuration must be done by the application.
> It should be the application choice to get its input from the command line,
> a CLI or a config file.
> 
Yes, that's exactly what I was thinking.

/Bruce

[PATCH v3 0/6] librte_cfgfile enhancements

[Allain Legacy]

This patchset includes some minor enhancements that we have developped for
our DPDK application.  We would like to contribute them upstream to help
ease adoption of the DPDK by anyone looking for this type of
functionality.  The commit logs on each patch should be self-sufficient in
explaining the intent and purpose.

v2:
* Added unit tests for the cfgfile library in the initial patch of the
  series and then added additional tests in subsequent patches where
  appropriate.  These will not run unless the following config parameter is
  set and additional packages are installed (e.g., libarchive-dev):
        CONFIG_RTE_APP_TEST_RESOURCE_TAR=y
* Reworked the configurable comment character patch to allow specifying a
  different character at runtime rather than build time.  Used a separate
  API to avoid affecting existing users or users that choose not to
  leverage the extended API.  Used a "parameters" structure to pass
  additional arguments rather than adding more arguments to the function to
  allow expansion in the future with minimal impact on existing users.
* Dropped the patch to initialize the cfg structure because the segfault
  that this was trying to address was already fixed by 2 earlier commits
  which we did not have in our development environment.  I realized this
  while trying to add unit tests to catch the segfault case.
* Fixed the doxygen comments related to the RTE_CFG_GLOBAL_SECTION patch
* Added an additional patch to allow parsing a key with an empty value
  (i.e., "key=").  I realized that I had forgotten to include this in my
  first patchset.

v3:
* Incorporated review feedback to validate comment chars
* Incorporated review feedback to keep flags as standalone argument instead
  of grouping it within the new params argument for comment characters
* Incorporated review feedback to remove the params initialization function
  and instead require users to initialize all fields to defaults on their
  own.
* Incorported review feedback to require a flag to allow empty value
  strings
* Adjusted unit tests to accommodate above changes.
* Reworded commit title to fix typo


Allain Legacy (5):
  test: basic unit tests for cfgfile
  cfgfile: add support for global properties section
  cfgfile: add support for configurable comment character
  cfgfile: use strnlen to constrain memchr search
  cfgfile: add support for empty value string

Joseph Richard (1):
  cfgfile: increase local buffer size for max name and value

 MAINTAINERS                                     |   1 +
 lib/librte_cfgfile/rte_cfgfile.c                | 102 +++++++-
 lib/librte_cfgfile/rte_cfgfile.h                |  45 +++-
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 327 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_comment.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 +
 test/test/test_cfgfiles/etc/sample2.ini         |  12 +
 13 files changed, 505 insertions(+), 10 deletions(-)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_comment.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

-- 
2.12.1

[PATCH v3 1/6] test: basic unit tests for cfgfile

[Allain Legacy]

This commit adds the basic infrastructure for the cfgfile library unit
tests.  It includes success path tests for the most commonly used APIs.
More unit tests will be added later.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 MAINTAINERS                                     |   1 +
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 210 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 ++
 9 files changed, 236 insertions(+)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini

diff --git a/MAINTAINERS b/MAINTAINERS
index 0b1524d3c..8eeb907a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -588,6 +588,7 @@ Other libraries
 Configuration file
 M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
 F: lib/librte_cfgfile/
+F: test/test/test_cfgfile*
 
 Interactive command line
 M: Olivier Matz <olivier.matz@6wind.com>
diff --git a/test/test/Makefile b/test/test/Makefile
index 79f0c6179..60c1761fa 100644
--- a/test/test/Makefile
+++ b/test/test/Makefile
@@ -85,6 +85,8 @@ $(eval $(call linked_resource,test_resource_c,test_resource.res))
 $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
 SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
 $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
+SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_cfgfile.c
+$(eval $(call linked_tar_resource,test_cfgfiles,test_cfgfiles))
 SRCS-y += test_prefetch.c
 SRCS-y += test_byteorder.c
 SRCS-y += test_per_lcore.c
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
new file mode 100644
index 000000000..faca89696
--- /dev/null
+++ b/test/test/test_cfgfile.c
@@ -0,0 +1,210 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Wind River Systems Inc. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_cfgfile.h>
+
+#include "test.h"
+#include "resource.h"
+
+
+#define CFG_FILES_ETC "test_cfgfiles/etc"
+
+REGISTER_LINKED_RESOURCE(test_cfgfiles);
+
+static int
+test_cfgfile_setup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_untar(r);
+	TEST_ASSERT_SUCCESS(ret, "failed to untar %s", r->name);
+
+	return 0;
+}
+
+static int
+test_cfgfile_cleanup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_rm_by_tar(r);
+	TEST_ASSERT_SUCCESS(ret, "Failed to delete resource %s", r->name);
+
+	return 0;
+}
+
+static int
+_test_cfgfile_sample(struct rte_cfgfile *cfgfile)
+{
+	const char *value;
+	int ret;
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 2, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key1");
+	TEST_ASSERT(strcmp("value1", value) == 0,
+		    "key1 unexpected value: %s", value);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section2");
+	TEST_ASSERT(ret, "section2 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section2");
+	TEST_ASSERT(ret == 2, "section2 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key2");
+	TEST_ASSERT(strcmp("value2", value) == 0,
+		    "key2 unexpected value: %s", value);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key3");
+	TEST_ASSERT(strcmp("value3", value) == 0,
+		    "key3 unexpected value: %s", value);
+
+	return 0;
+}
+
+static int
+test_cfgfile_sample1(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/sample1.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_section_header(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_missing_section(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_empty_file(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 0, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile(void)
+{
+	if (test_cfgfile_setup())
+		return -1;
+
+	if (test_cfgfile_sample1())
+		return -1;
+
+	if (test_cfgfile_invalid_section_header())
+		return -1;
+
+	if (test_cfgfile_invalid_key_value_pair())
+		return -1;
+
+	if (test_cfgfile_missing_section())
+		return -1;
+
+	if (test_cfgfile_empty_file())
+		return -1;
+
+	if (test_cfgfile_cleanup())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(cfgfile_autotest, test_cfgfile);
diff --git a/test/test/test_cfgfiles/etc/empty.ini b/test/test/test_cfgfiles/etc/empty.ini
new file mode 100644
index 000000000..e69de29bb
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
new file mode 100644
index 000000000..8f203ce3e
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+invalid=
diff --git a/test/test/test_cfgfiles/etc/invalid_section.ini b/test/test/test_cfgfiles/etc/invalid_section.ini
new file mode 100644
index 000000000..95d680397
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/invalid_section.ini
@@ -0,0 +1,3 @@
+[invalid
+; this is section 1
+key1=value1
diff --git a/test/test/test_cfgfiles/etc/line_too_long.ini b/test/test/test_cfgfiles/etc/line_too_long.ini
new file mode 100644
index 000000000..1dce16483
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/line_too_long.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
diff --git a/test/test/test_cfgfiles/etc/missing_section.ini b/test/test/test_cfgfiles/etc/missing_section.ini
new file mode 100644
index 000000000..c78e131b4
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/missing_section.ini
@@ -0,0 +1,2 @@
+; no section
+key=value
diff --git a/test/test/test_cfgfiles/etc/sample1.ini b/test/test/test_cfgfiles/etc/sample1.ini
new file mode 100644
index 000000000..aef91c241
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample1.ini
@@ -0,0 +1,12 @@
+; this is a global comment
+
+[section1]
+; this is section 1
+key1=value1
+
+[section2]
+; this is section 2
+;key1=value1
+key2=value2
+key3=value3 ; this is key3
+ignore-missing-separator
-- 
2.12.1

[PATCH v3 3/6] cfgfile: add support for configurable comment character

[Allain Legacy]

The current cfgfile comment character is hardcoded to ';'.  This commit a
new API to allow the user to specify which comment character to use while
parsing the file.

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.

The approach of using a new API with an extensible parameters structure was
used rather than simply adding a new argument to the existing API to allow
for additional arguments to be introduced in the future.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c        | 61 ++++++++++++++++++++++++++++++++-
 lib/librte_cfgfile/rte_cfgfile.h        | 29 ++++++++++++++++
 test/test/test_cfgfile.c                | 47 +++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/sample2.ini | 12 +++++++
 4 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 832fea829..63e34bbb0 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -35,6 +35,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
+#include <rte_common.h>
 #include <rte_string_fns.h>
 
 #include "rte_cfgfile.h"
@@ -58,6 +59,25 @@ struct rte_cfgfile {
  * for new entries do we add in */
 #define CFG_ALLOC_ENTRY_BATCH 16
 
+/**
+ * Default cfgfile load parameters.
+ */
+static const struct rte_cfgfile_parameters default_cfgfile_params = {
+	.comment_character = CFG_DEFAULT_COMMENT_CHARACTER,
+};
+
+/**
+ * Defines the list of acceptable comment characters supported by this
+ * library.
+ */
+static const char valid_comment_chars[] = {
+	'!',
+	'#',
+	'%',
+	';',
+	'@'
+};
+
 static unsigned
 _strip(char *str, unsigned len)
 {
@@ -85,9 +105,45 @@ _strip(char *str, unsigned len)
 	return newlen;
 }
 
+static int
+rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
+{
+	unsigned int valid_comment;
+	unsigned int i;
+
+	if (!params) {
+		printf("Error - missing cfgfile parameters\n");
+		return -EINVAL;
+	}
+
+	valid_comment = 0;
+	for (i = 0; i < RTE_DIM(valid_comment_chars); i++) {
+		if (params->comment_character == valid_comment_chars[i]) {
+			valid_comment = 1;
+			break;
+		}
+	}
+
+	if (valid_comment == 0)	{
+		printf("Error - invalid comment characters %c\n",
+		       params->comment_character);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 struct rte_cfgfile *
 rte_cfgfile_load(const char *filename, int flags)
 {
+	return rte_cfgfile_load_with_params(filename, flags,
+					    &default_cfgfile_params);
+}
+
+struct rte_cfgfile *
+rte_cfgfile_load_with_params(const char *filename, int flags,
+			     const struct rte_cfgfile_parameters *params)
+{
 	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
 	int allocated_entries = 0;
 	int curr_section = -1;
@@ -96,6 +152,9 @@ rte_cfgfile_load(const char *filename, int flags)
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
+	if (rte_cfgfile_check_params(params))
+		return NULL;
+
 	FILE *f = fopen(filename, "r");
 	if (f == NULL)
 		return NULL;
@@ -132,7 +191,7 @@ rte_cfgfile_load(const char *filename, int flags)
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, ';', sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, sizeof(buffer));
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 0e805c268..a7cd3944e 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/** Configuration file operation optional arguments */
+struct rte_cfgfile_parameters {
+	char comment_character;
+	/**< Config file comment character; one of '!', '#', '%', ';', '@' */
+};
+
 /**@{ cfgfile load operation flags */
 /**
  * Indicates that the file supports key value entries before the first defined
@@ -74,6 +80,9 @@ struct rte_cfgfile_entry {
 #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
 /**@} */
 
+/** Defines the default comment character used for parsing config files. */
+#define CFG_DEFAULT_COMMENT_CHARACTER ';'
+
 /**
 * Open config file
 *
@@ -87,6 +96,26 @@ struct rte_cfgfile_entry {
 struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
 
 /**
+ * Open config file with specified optional parameters.  Use @see
+ * rte_cfgfile_init_parameters to setup the default parameters.
+ *
+ * @param filename
+ *   Config file name
+ * @param flags
+ *   Config file flags
+ * @param params
+ *   Additional configuration attributes.  Must be configured with default
+ *   values prior to invoking this API.
+ * @return
+ *   Handle to configuration file on success, NULL otherwise
+ * @param
+ *
+ */
+struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
+	int flags, const struct rte_cfgfile_parameters *params);
+
+
+/**
 * Get number of sections in config file
 *
 * @param cfg
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index ad293cc89..0478de59e 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -130,6 +130,30 @@ test_cfgfile_sample1(void)
 }
 
 static int
+test_cfgfile_sample2(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	/* override comment character */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '#';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_section_header(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -141,6 +165,23 @@ test_cfgfile_invalid_section_header(void)
 }
 
 static int
+test_cfgfile_invalid_comment(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+
+	/* override comment character with an invalid one */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '$';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_key_value_pair(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -219,9 +260,15 @@ test_cfgfile(void)
 	if (test_cfgfile_sample1())
 		return -1;
 
+	if (test_cfgfile_sample2())
+		return -1;
+
 	if (test_cfgfile_invalid_section_header())
 		return -1;
 
+	if (test_cfgfile_invalid_comment())
+		return -1;
+
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/sample2.ini b/test/test/test_cfgfiles/etc/sample2.ini
new file mode 100644
index 000000000..21075e976
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample2.ini
@@ -0,0 +1,12 @@
+# this is a global comment
+
+[section1]
+# this is section 1
+key1=value1
+
+[section2]
+# this is section 2
+#key1=value1
+key2=value2
+key3=value3 # this is key3
+ignore-missing-separator
-- 
2.12.1

[PATCH v3 4/6] cfgfile: use strnlen to constrain memchr search

[Allain Legacy]

The call to memchr() uses the absolute length of the string buffer instead
of the actual length of the string returned by fgets().  This causes the
search to go beyond the '\n' character and find ';' characters in random
garbage on the stack.  This then causes the 'len' variable to be updated
and the subsequent search for the '=' character to potentially find one
beyond the first newline character.

Since this bug relies on ';' and '=' characters appearing in random places
in the 'buffer' variable it is intermittently reproducible at best.

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

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 63e34bbb0..e4a3885b7 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -191,7 +191,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, params->comment_character, sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, len);
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
-- 
2.12.1

[PATCH v3 6/6] cfgfile: add support for empty value string

[Allain Legacy]

This commit adds support to the cfgfile library for parsing a key=value
line that has no value string specified (e.g., "key=").  This can be used
to override a configuration attribute that has a default value or default
list of values to set it back to an undefined value to disable
functionality.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c                | 23 ++++++++++++------
 lib/librte_cfgfile/rte_cfgfile.h                |  6 +++++
 test/test/test_cfgfile.c                        | 32 +++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty_key_value.ini |  2 +-
 4 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 4ef7decb3..b54a523d2 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -258,12 +258,21 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 
 			struct rte_cfgfile_section *sect =
 				cfg->sections[curr_section];
-			char *split[2];
-			if (rte_strsplit(buffer, sizeof(buffer), split, 2, '=')
-				!= 2) {
-				printf("Error at line %d - cannot split "
-					"string\n", lineno);
-				goto error1;
+			int n;
+			char *split[2] = {NULL};
+			n = rte_strsplit(buffer, sizeof(buffer), split, 2, '=');
+			if (flags & CFG_FLAG_EMPTY_VALUES) {
+				if ((n < 1) || (n > 2)) {
+					printf("Error at line %d - cannot split string, n=%d\n",
+					       lineno, n);
+					goto error1;
+				}
+			} else {
+				if (n != 2) {
+					printf("Error at line %d - cannot split string, n=%d\n",
+					       lineno, n);
+					goto error1;
+				}
 			}
 
 			curr_entry++;
@@ -293,7 +302,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 			snprintf(entry->name, sizeof(entry->name), "%s",
 				split[0]);
 			snprintf(entry->value, sizeof(entry->value), "%s",
-				split[1]);
+				 split[1] ? split[1] : "");
 			_strip(entry->name, strnlen(entry->name,
 				sizeof(entry->name)));
 			_strip(entry->value, strnlen(entry->value,
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index a7cd3944e..b06f55d56 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -78,6 +78,12 @@ struct rte_cfgfile_parameters {
  * section.  These entries can be accessed in the "GLOBAL" section.
  */
 #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
+
+/**
+ * Indicates that file supports key value entries where the value can be zero
+ * length (e.g., "key=").
+ */
+#define CFG_FLAG_EMPTY_VALUES (1 << 1)
 /**@} */
 
 /** Defines the default comment character used for parsing config files. */
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index 0478de59e..4559f8688 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -193,6 +193,35 @@ test_cfgfile_invalid_key_value_pair(void)
 }
 
 static int
+test_cfgfile_empty_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+	const char *value;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini",
+				   CFG_FLAG_EMPTY_VALUES);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse empty_key_value.ini");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key");
+	TEST_ASSERT(strlen(value) == 0, "key unexpected value: %s", value);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_missing_section(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -272,6 +301,9 @@ test_cfgfile(void)
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
+	if (test_cfgfile_empty_key_value_pair())
+		return -1;
+
 	if (test_cfgfile_missing_section())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
index 8f203ce3e..53284467b 100644
--- a/test/test/test_cfgfiles/etc/empty_key_value.ini
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -1,3 +1,3 @@
 [section1]
 ; this is section 1
-invalid=
+key=
-- 
2.12.1

[PATCH v4 0/6] librte_cfgfile enhancements

[Allain Legacy]

This patchset includes some minor enhancements that we have developped for
our DPDK application.  We would like to contribute them upstream to help
ease adoption of the DPDK by anyone looking for this type of
functionality.  The commit logs on each patch should be self-sufficient in
explaining the intent and purpose.

v2:
* Added unit tests for the cfgfile library in the initial patch of the
  series and then added additional tests in subsequent patches where
  appropriate.  These will not run unless the following config parameter is
  set and additional packages are installed (e.g., libarchive-dev):
        CONFIG_RTE_APP_TEST_RESOURCE_TAR=y
* Reworked the configurable comment character patch to allow specifying a
  different character at runtime rather than build time.  Used a separate
  API to avoid affecting existing users or users that choose not to
  leverage the extended API.  Used a "parameters" structure to pass
  additional arguments rather than adding more arguments to the function to
  allow expansion in the future with minimal impact on existing users.
* Dropped the patch to initialize the cfg structure because the segfault
  that this was trying to address was already fixed by 2 earlier commits
  which we did not have in our development environment.  I realized this
  while trying to add unit tests to catch the segfault case.
* Fixed the doxygen comments related to the RTE_CFG_GLOBAL_SECTION patch
* Added an additional patch to allow parsing a key with an empty value
  (i.e., "key=").  I realized that I had forgotten to include this in my
  first patchset.

v3:
* Incorporated review feedback to validate comment chars
* Incorporated review feedback to keep flags as standalone argument instead
  of grouping it within the new params argument for comment characters
* Incorporated review feedback to remove the params initialization function
  and instead require users to initialize all fields to defaults on their
  own.
* Incorported review feedback to require a flag to allow empty value
  strings
* Adjusted unit tests to accommodate above changes.
* Reworded commit title to fix typo "unamed"
* NOTE: This patchset version was incomplete on dpdk.org because of an email
  spam filter issue.

v4:
* Changed flag defines to unnamed enum
* Removed comment references to rte_cfgfile_init_params() since it was
  removed from the implementation
* Moved field comments to above definition instead of below in the
  rte_cfgfile_parameters definition

Allain Legacy (5):
  test: basic unit tests for cfgfile
  cfgfile: add support for global properties section
  cfgfile: add support for configurable comment character
  cfgfile: use strnlen to constrain memchr search
  cfgfile: add support for empty value string

Joseph Richard (1):
  cfgfile: increase local buffer size for max name and value

 MAINTAINERS                                     |   1 +
 lib/librte_cfgfile/rte_cfgfile.c                | 102 +++++++-
 lib/librte_cfgfile/rte_cfgfile.h                |  45 +++-
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 322 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 +
 test/test/test_cfgfiles/etc/sample2.ini         |  12 +
 12 files changed, 497 insertions(+), 10 deletions(-)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

-- 
2.12.1

[PATCH v4 1/6] test: basic unit tests for cfgfile

[Allain Legacy]

This commit adds the basic infrastructure for the cfgfile library unit
tests.  It includes success path tests for the most commonly used APIs.
More unit tests will be added later.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 MAINTAINERS                                     |   1 +
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 210 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 ++
 9 files changed, 236 insertions(+)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini

diff --git a/MAINTAINERS b/MAINTAINERS
index 4cb6e4957..0eab1681d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -589,6 +589,7 @@ Other libraries
 Configuration file
 M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
 F: lib/librte_cfgfile/
+F: test/test/test_cfgfile*
 
 Interactive command line
 M: Olivier Matz <olivier.matz@6wind.com>
diff --git a/test/test/Makefile b/test/test/Makefile
index 79f0c6179..60c1761fa 100644
--- a/test/test/Makefile
+++ b/test/test/Makefile
@@ -85,6 +85,8 @@ $(eval $(call linked_resource,test_resource_c,test_resource.res))
 $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
 SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
 $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
+SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_cfgfile.c
+$(eval $(call linked_tar_resource,test_cfgfiles,test_cfgfiles))
 SRCS-y += test_prefetch.c
 SRCS-y += test_byteorder.c
 SRCS-y += test_per_lcore.c
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
new file mode 100644
index 000000000..faca89696
--- /dev/null
+++ b/test/test/test_cfgfile.c
@@ -0,0 +1,210 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Wind River Systems Inc. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_cfgfile.h>
+
+#include "test.h"
+#include "resource.h"
+
+
+#define CFG_FILES_ETC "test_cfgfiles/etc"
+
+REGISTER_LINKED_RESOURCE(test_cfgfiles);
+
+static int
+test_cfgfile_setup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_untar(r);
+	TEST_ASSERT_SUCCESS(ret, "failed to untar %s", r->name);
+
+	return 0;
+}
+
+static int
+test_cfgfile_cleanup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_rm_by_tar(r);
+	TEST_ASSERT_SUCCESS(ret, "Failed to delete resource %s", r->name);
+
+	return 0;
+}
+
+static int
+_test_cfgfile_sample(struct rte_cfgfile *cfgfile)
+{
+	const char *value;
+	int ret;
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 2, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key1");
+	TEST_ASSERT(strcmp("value1", value) == 0,
+		    "key1 unexpected value: %s", value);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section2");
+	TEST_ASSERT(ret, "section2 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section2");
+	TEST_ASSERT(ret == 2, "section2 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key2");
+	TEST_ASSERT(strcmp("value2", value) == 0,
+		    "key2 unexpected value: %s", value);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key3");
+	TEST_ASSERT(strcmp("value3", value) == 0,
+		    "key3 unexpected value: %s", value);
+
+	return 0;
+}
+
+static int
+test_cfgfile_sample1(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/sample1.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_section_header(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_missing_section(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_empty_file(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 0, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile(void)
+{
+	if (test_cfgfile_setup())
+		return -1;
+
+	if (test_cfgfile_sample1())
+		return -1;
+
+	if (test_cfgfile_invalid_section_header())
+		return -1;
+
+	if (test_cfgfile_invalid_key_value_pair())
+		return -1;
+
+	if (test_cfgfile_missing_section())
+		return -1;
+
+	if (test_cfgfile_empty_file())
+		return -1;
+
+	if (test_cfgfile_cleanup())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(cfgfile_autotest, test_cfgfile);
diff --git a/test/test/test_cfgfiles/etc/empty.ini b/test/test/test_cfgfiles/etc/empty.ini
new file mode 100644
index 000000000..e69de29bb
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
new file mode 100644
index 000000000..8f203ce3e
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+invalid=
diff --git a/test/test/test_cfgfiles/etc/invalid_section.ini b/test/test/test_cfgfiles/etc/invalid_section.ini
new file mode 100644
index 000000000..95d680397
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/invalid_section.ini
@@ -0,0 +1,3 @@
+[invalid
+; this is section 1
+key1=value1
diff --git a/test/test/test_cfgfiles/etc/line_too_long.ini b/test/test/test_cfgfiles/etc/line_too_long.ini
new file mode 100644
index 000000000..1dce16483
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/line_too_long.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
diff --git a/test/test/test_cfgfiles/etc/missing_section.ini b/test/test/test_cfgfiles/etc/missing_section.ini
new file mode 100644
index 000000000..c78e131b4
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/missing_section.ini
@@ -0,0 +1,2 @@
+; no section
+key=value
diff --git a/test/test/test_cfgfiles/etc/sample1.ini b/test/test/test_cfgfiles/etc/sample1.ini
new file mode 100644
index 000000000..aef91c241
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample1.ini
@@ -0,0 +1,12 @@
+; this is a global comment
+
+[section1]
+; this is section 1
+key1=value1
+
+[section2]
+; this is section 2
+;key1=value1
+key2=value2
+key3=value3 ; this is key3
+ignore-missing-separator
-- 
2.12.1

[PATCH v4 2/6] cfgfile: add support for global properties section

[Allain Legacy]

The current implementation of the cfgfile library requires that all
key=value pairs be within [SECTION] definitions.  The ini file standard
allows for key=value pairs in an unnamed section.

   https://en.wikipedia.org/wiki/INI_file#Global_properties

This commit adds the capability of parsing key=value pairs from such an
unnamed section. The CFG_FLAG_GLOBAL_SECTION flag must be passed to the
rte_cfgfile_load() API to enable this functionality.  Any key=value pairs
found before the first section can be accessed in the section named
"GLOBAL".

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 16 ++++++++++++++++
 lib/librte_cfgfile/rte_cfgfile.h | 13 ++++++++++++-
 test/test/test_cfgfile.c         | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 829109a77..832fea829 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -107,6 +107,22 @@ rte_cfgfile_load(const char *filename, int flags)
 
 	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
 
+	if (flags & CFG_FLAG_GLOBAL_SECTION) {
+		curr_section = 0;
+		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
+		cfg->sections[curr_section] = malloc(
+			sizeof(*cfg->sections[0]) +
+			sizeof(cfg->sections[0]->entries[0]) *
+			allocated_entries);
+		if (cfg->sections[curr_section] == NULL) {
+			printf("Error - no memory for global section\n");
+			goto error1;
+		}
+
+		snprintf(cfg->sections[curr_section]->name,
+				 sizeof(cfg->sections[0]->name), "GLOBAL");
+	}
+
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
 		size_t len = strnlen(buffer, sizeof(buffer));
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index b40e6a135..2dadd75d4 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,13 +66,24 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/**@{ cfgfile load operation flags */
+enum {
+	/**
+	 * Indicates that the file supports key value entries before the first
+	 * defined section.  These entries can be accessed in the "GLOBAL"
+	 * section.
+	 */
+	CFG_FLAG_GLOBAL_SECTION = 1,
+};
+/**@} */
+
 /**
 * Open config file
 *
 * @param filename
 *   Config file name
 * @param flags
-*   Config file flags, Reserved for future use. Must be set to 0.
+*   Config file flags
 * @return
 *   Handle to configuration file on success, NULL otherwise
 */
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index faca89696..a7de3706c 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -163,6 +163,36 @@ test_cfgfile_missing_section(void)
 }
 
 static int
+test_cfgfile_global_properties(void)
+{
+	struct rte_cfgfile *cfgfile;
+	const char *value;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini",
+				   CFG_FLAG_GLOBAL_SECTION);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "GLOBAL");
+	TEST_ASSERT(ret, "global section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "GLOBAL");
+	TEST_ASSERT(ret == 1, "GLOBAL unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "GLOBAL", "key");
+	TEST_ASSERT(strcmp("value", value) == 0,
+		    "key unexpected value: %s", value);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_empty_file(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -198,6 +228,9 @@ test_cfgfile(void)
 	if (test_cfgfile_missing_section())
 		return -1;
 
+	if (test_cfgfile_global_properties())
+		return -1;
+
 	if (test_cfgfile_empty_file())
 		return -1;
 
-- 
2.12.1

[PATCH v4 3/6] cfgfile: add support for configurable comment character

[Allain Legacy]

The current cfgfile comment character is hardcoded to ';'.  This commit a
new API to allow the user to specify which comment character to use while
parsing the file.

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.

The approach of using a new API with an extensible parameters structure was
used rather than simply adding a new argument to the existing API to allow
for additional arguments to be introduced in the future.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c        | 61 ++++++++++++++++++++++++++++++++-
 lib/librte_cfgfile/rte_cfgfile.h        | 28 +++++++++++++++
 test/test/test_cfgfile.c                | 47 +++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/sample2.ini | 12 +++++++
 4 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 832fea829..63e34bbb0 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -35,6 +35,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
+#include <rte_common.h>
 #include <rte_string_fns.h>
 
 #include "rte_cfgfile.h"
@@ -58,6 +59,25 @@ struct rte_cfgfile {
  * for new entries do we add in */
 #define CFG_ALLOC_ENTRY_BATCH 16
 
+/**
+ * Default cfgfile load parameters.
+ */
+static const struct rte_cfgfile_parameters default_cfgfile_params = {
+	.comment_character = CFG_DEFAULT_COMMENT_CHARACTER,
+};
+
+/**
+ * Defines the list of acceptable comment characters supported by this
+ * library.
+ */
+static const char valid_comment_chars[] = {
+	'!',
+	'#',
+	'%',
+	';',
+	'@'
+};
+
 static unsigned
 _strip(char *str, unsigned len)
 {
@@ -85,9 +105,45 @@ _strip(char *str, unsigned len)
 	return newlen;
 }
 
+static int
+rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
+{
+	unsigned int valid_comment;
+	unsigned int i;
+
+	if (!params) {
+		printf("Error - missing cfgfile parameters\n");
+		return -EINVAL;
+	}
+
+	valid_comment = 0;
+	for (i = 0; i < RTE_DIM(valid_comment_chars); i++) {
+		if (params->comment_character == valid_comment_chars[i]) {
+			valid_comment = 1;
+			break;
+		}
+	}
+
+	if (valid_comment == 0)	{
+		printf("Error - invalid comment characters %c\n",
+		       params->comment_character);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 struct rte_cfgfile *
 rte_cfgfile_load(const char *filename, int flags)
 {
+	return rte_cfgfile_load_with_params(filename, flags,
+					    &default_cfgfile_params);
+}
+
+struct rte_cfgfile *
+rte_cfgfile_load_with_params(const char *filename, int flags,
+			     const struct rte_cfgfile_parameters *params)
+{
 	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
 	int allocated_entries = 0;
 	int curr_section = -1;
@@ -96,6 +152,9 @@ rte_cfgfile_load(const char *filename, int flags)
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
+	if (rte_cfgfile_check_params(params))
+		return NULL;
+
 	FILE *f = fopen(filename, "r");
 	if (f == NULL)
 		return NULL;
@@ -132,7 +191,7 @@ rte_cfgfile_load(const char *filename, int flags)
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, ';', sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, sizeof(buffer));
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 2dadd75d4..41d66b829 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/** Configuration file operation optional arguments */
+struct rte_cfgfile_parameters {
+	/** Config file comment character; one of '!', '#', '%', ';', '@' */
+	char comment_character;
+};
+
 /**@{ cfgfile load operation flags */
 enum {
 	/**
@@ -77,6 +83,9 @@ enum {
 };
 /**@} */
 
+/** Defines the default comment character used for parsing config files. */
+#define CFG_DEFAULT_COMMENT_CHARACTER ';'
+
 /**
 * Open config file
 *
@@ -90,6 +99,25 @@ enum {
 struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
 
 /**
+ * Open config file with specified optional parameters.
+ *
+ * @param filename
+ *   Config file name
+ * @param flags
+ *   Config file flags
+ * @param params
+ *   Additional configuration attributes.  Must be configured with desired
+ *   values prior to invoking this API.
+ * @return
+ *   Handle to configuration file on success, NULL otherwise
+ * @param
+ *
+ */
+struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
+	int flags, const struct rte_cfgfile_parameters *params);
+
+
+/**
 * Get number of sections in config file
 *
 * @param cfg
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index a7de3706c..8eabd711c 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -130,6 +130,30 @@ test_cfgfile_sample1(void)
 }
 
 static int
+test_cfgfile_sample2(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	/* override comment character */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '#';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_section_header(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -141,6 +165,23 @@ test_cfgfile_invalid_section_header(void)
 }
 
 static int
+test_cfgfile_invalid_comment(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+
+	/* override comment character with an invalid one */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '$';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_key_value_pair(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -219,9 +260,15 @@ test_cfgfile(void)
 	if (test_cfgfile_sample1())
 		return -1;
 
+	if (test_cfgfile_sample2())
+		return -1;
+
 	if (test_cfgfile_invalid_section_header())
 		return -1;
 
+	if (test_cfgfile_invalid_comment())
+		return -1;
+
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/sample2.ini b/test/test/test_cfgfiles/etc/sample2.ini
new file mode 100644
index 000000000..21075e976
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample2.ini
@@ -0,0 +1,12 @@
+# this is a global comment
+
+[section1]
+# this is section 1
+key1=value1
+
+[section2]
+# this is section 2
+#key1=value1
+key2=value2
+key3=value3 # this is key3
+ignore-missing-separator
-- 
2.12.1

Re: [PATCH v4 3/6] cfgfile: add support for configurable comment character

[Thomas Monjalon]

Hi,

2017-03-30 14:54, Allain Legacy:
>  /**
> + * Open config file with specified optional parameters.
> + *
> + * @param filename
> + *   Config file name
> + * @param flags
> + *   Config file flags
> + * @param params
> + *   Additional configuration attributes.  Must be configured with desired
> + *   values prior to invoking this API.
> + * @return
> + *   Handle to configuration file on success, NULL otherwise
> + * @param

This @param should be removed.

> + *
> + */
> +struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
> +       int flags, const struct rte_cfgfile_parameters *params);
> 

The new public function must be exported in the map file
in a 17.05 section.
Otherwise link with the .so fails:
	undefined reference to `rte_cfgfile_load_with_params'

Re: [PATCH v4 3/6] cfgfile: add support for configurable comment character

[Legacy, Allain]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, March 31, 2017 6:08 AM
<...> 
> > + * @return
> > + *   Handle to configuration file on success, NULL otherwise
> > + * @param
> 
> This @param should be removed.

Ok.  I will remove it.

> 
> > + *
> > + */
> > +struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
> > +       int flags, const struct rte_cfgfile_parameters *params);
> >
> 
> The new public function must be exported in the map file in a 17.05 section.
> Otherwise link with the .so fails:
> 	undefined reference to `rte_cfgfile_load_with_params'

Ok.  I will send a v5 shortly.

[PATCH v4 4/6] cfgfile: use strnlen to constrain memchr search

[Allain Legacy]

The call to memchr() uses the absolute length of the string buffer instead
of the actual length of the string returned by fgets().  This causes the
search to go beyond the '\n' character and find ';' characters in random
garbage on the stack.  This then causes the 'len' variable to be updated
and the subsequent search for the '=' character to potentially find one
beyond the first newline character.

Since this bug relies on ';' and '=' characters appearing in random places
in the 'buffer' variable it is intermittently reproducible at best.

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

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 63e34bbb0..e4a3885b7 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -191,7 +191,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, params->comment_character, sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, len);
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
-- 
2.12.1

[PATCH v4 5/6] cfgfile: increase local buffer size for max name and value

[Allain Legacy]

From: Joseph Richard <joseph.richard@windriver.com>

When parsing a ini file with a "key = value" line that has both "key" and
"value" sized to the maximum allowed length causes a parsing failure.  The
internal "buffer" variable should be sized at least as large as the maximum
for both fields.  This commit updates the local array to be sized to hold
the max name, max value, " = ", and the nul terminator.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index e4a3885b7..4ef7decb3 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -148,7 +148,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 	int allocated_entries = 0;
 	int curr_section = -1;
 	int curr_entry = -1;
-	char buffer[256] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
-- 
2.12.1

[PATCH v4 6/6] cfgfile: add support for empty value string

[Allain Legacy]

This commit adds support to the cfgfile library for parsing a key=value
line that has no value string specified (e.g., "key=").  This can be used
to override a configuration attribute that has a default value or default
list of values to set it back to an undefined value to disable
functionality.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c                | 23 ++++++++++++------
 lib/librte_cfgfile/rte_cfgfile.h                |  6 +++++
 test/test/test_cfgfile.c                        | 32 +++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty_key_value.ini |  2 +-
 4 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 4ef7decb3..b54a523d2 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -258,12 +258,21 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 
 			struct rte_cfgfile_section *sect =
 				cfg->sections[curr_section];
-			char *split[2];
-			if (rte_strsplit(buffer, sizeof(buffer), split, 2, '=')
-				!= 2) {
-				printf("Error at line %d - cannot split "
-					"string\n", lineno);
-				goto error1;
+			int n;
+			char *split[2] = {NULL};
+			n = rte_strsplit(buffer, sizeof(buffer), split, 2, '=');
+			if (flags & CFG_FLAG_EMPTY_VALUES) {
+				if ((n < 1) || (n > 2)) {
+					printf("Error at line %d - cannot split string, n=%d\n",
+					       lineno, n);
+					goto error1;
+				}
+			} else {
+				if (n != 2) {
+					printf("Error at line %d - cannot split string, n=%d\n",
+					       lineno, n);
+					goto error1;
+				}
 			}
 
 			curr_entry++;
@@ -293,7 +302,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 			snprintf(entry->name, sizeof(entry->name), "%s",
 				split[0]);
 			snprintf(entry->value, sizeof(entry->value), "%s",
-				split[1]);
+				 split[1] ? split[1] : "");
 			_strip(entry->name, strnlen(entry->name,
 				sizeof(entry->name)));
 			_strip(entry->value, strnlen(entry->value,
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 41d66b829..34d63cc9b 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -80,6 +80,12 @@ enum {
 	 * section.
 	 */
 	CFG_FLAG_GLOBAL_SECTION = 1,
+
+	/**
+	 * Indicates that file supports key value entries where the value can
+	 * be zero length (e.g., "key=").
+	 */
+	CFG_FLAG_EMPTY_VALUES = 2,
 };
 /**@} */
 
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index 8eabd711c..4cc9b14dd 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -193,6 +193,35 @@ test_cfgfile_invalid_key_value_pair(void)
 }
 
 static int
+test_cfgfile_empty_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+	const char *value;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini",
+				   CFG_FLAG_EMPTY_VALUES);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse empty_key_value.ini");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key");
+	TEST_ASSERT(strlen(value) == 0, "key unexpected value: %s", value);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_missing_section(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -272,6 +301,9 @@ test_cfgfile(void)
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
+	if (test_cfgfile_empty_key_value_pair())
+		return -1;
+
 	if (test_cfgfile_missing_section())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
index 8f203ce3e..53284467b 100644
--- a/test/test/test_cfgfiles/etc/empty_key_value.ini
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -1,3 +1,3 @@
 [section1]
 ; this is section 1
-invalid=
+key=
-- 
2.12.1

Re: [PATCH v4 0/6] librte_cfgfile enhancements

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Thursday, March 30, 2017 7:54 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com;
> thomas.monjalon@6wind.com
> Subject: [PATCH v4 0/6] librte_cfgfile enhancements
> 
> This patchset includes some minor enhancements that we have developped
> for
> our DPDK application.  We would like to contribute them upstream to help
> ease adoption of the DPDK by anyone looking for this type of
> functionality.  The commit logs on each patch should be self-sufficient in
> explaining the intent and purpose.
> 

Thanks, Allain!

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

[PATCH v5 0/6] librte_cfgfile enhancements

[Allain Legacy]

This patchset includes some minor enhancements that we have developped for
our DPDK application.  We would like to contribute them upstream to help
ease adoption of the DPDK by anyone looking for this type of
functionality.  The commit logs on each patch should be self-sufficient in
explaining the intent and purpose.

v2:
* Added unit tests for the cfgfile library in the initial patch of the
  series and then added additional tests in subsequent patches where
  appropriate.  These will not run unless the following config parameter is
  set and additional packages are installed (e.g., libarchive-dev):
        CONFIG_RTE_APP_TEST_RESOURCE_TAR=y
* Reworked the configurable comment character patch to allow specifying a
  different character at runtime rather than build time.  Used a separate
  API to avoid affecting existing users or users that choose not to
  leverage the extended API.  Used a "parameters" structure to pass
  additional arguments rather than adding more arguments to the function to
  allow expansion in the future with minimal impact on existing users.
* Dropped the patch to initialize the cfg structure because the segfault
  that this was trying to address was already fixed by 2 earlier commits
  which we did not have in our development environment.  I realized this
  while trying to add unit tests to catch the segfault case.
* Fixed the doxygen comments related to the RTE_CFG_GLOBAL_SECTION patch
* Added an additional patch to allow parsing a key with an empty value
  (i.e., "key=").  I realized that I had forgotten to include this in my
  first patchset.

v3:
* Incorporated review feedback to validate comment chars
* Incorporated review feedback to keep flags as standalone argument instead
  of grouping it within the new params argument for comment characters
* Incorporated review feedback to remove the params initialization function
  and instead require users to initialize all fields to defaults on their
  own.
* Incorported review feedback to require a flag to allow empty value
  strings
* Adjusted unit tests to accommodate above changes.
* Reworded commit title to fix typo "unamed"
* NOTE: This patchset version was incomplete on dpdk.org because of an
  email spam filter issue.

v4:
* Changed flag defines to unnamed enum
* Removed comment references to rte_cfgfile_init_params() since it was
  removed from the implementation
* Moved field comments to above definition instead of below in the
  rte_cfgfile_parameters definition

v5:
* Removed stray @param notation in rte_cfgfile.h file
* Updated rte_cfgfile_version.map file to add new public API


Allain Legacy (5):
  test: basic unit tests for cfgfile
  cfgfile: add support for global properties section
  cfgfile: add support for configurable comment character
  cfgfile: use strnlen to constrain memchr search
  cfgfile: add support for empty value string

Joseph Richard (1):
  cfgfile: increase local buffer size for max name and value

 MAINTAINERS                                     |   1 +
 lib/librte_cfgfile/rte_cfgfile.c                | 102 +++++++-
 lib/librte_cfgfile/rte_cfgfile.h                |  44 +++-
 lib/librte_cfgfile/rte_cfgfile_version.map      |   7 +
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 322 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 +
 test/test/test_cfgfiles/etc/sample2.ini         |  12 +
 13 files changed, 503 insertions(+), 10 deletions(-)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

-- 
2.12.1

[PATCH v5 1/6] test: basic unit tests for cfgfile

[Allain Legacy]

This commit adds the basic infrastructure for the cfgfile library unit
tests.  It includes success path tests for the most commonly used APIs.
More unit tests will be added later.

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 MAINTAINERS                                     |   1 +
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 210 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 ++
 9 files changed, 236 insertions(+)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini

diff --git a/MAINTAINERS b/MAINTAINERS
index 4cb6e4957..0eab1681d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -589,6 +589,7 @@ Other libraries
 Configuration file
 M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
 F: lib/librte_cfgfile/
+F: test/test/test_cfgfile*
 
 Interactive command line
 M: Olivier Matz <olivier.matz@6wind.com>
diff --git a/test/test/Makefile b/test/test/Makefile
index 79f0c6179..60c1761fa 100644
--- a/test/test/Makefile
+++ b/test/test/Makefile
@@ -85,6 +85,8 @@ $(eval $(call linked_resource,test_resource_c,test_resource.res))
 $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
 SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
 $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
+SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_cfgfile.c
+$(eval $(call linked_tar_resource,test_cfgfiles,test_cfgfiles))
 SRCS-y += test_prefetch.c
 SRCS-y += test_byteorder.c
 SRCS-y += test_per_lcore.c
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
new file mode 100644
index 000000000..faca89696
--- /dev/null
+++ b/test/test/test_cfgfile.c
@@ -0,0 +1,210 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Wind River Systems Inc. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_cfgfile.h>
+
+#include "test.h"
+#include "resource.h"
+
+
+#define CFG_FILES_ETC "test_cfgfiles/etc"
+
+REGISTER_LINKED_RESOURCE(test_cfgfiles);
+
+static int
+test_cfgfile_setup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_untar(r);
+	TEST_ASSERT_SUCCESS(ret, "failed to untar %s", r->name);
+
+	return 0;
+}
+
+static int
+test_cfgfile_cleanup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_rm_by_tar(r);
+	TEST_ASSERT_SUCCESS(ret, "Failed to delete resource %s", r->name);
+
+	return 0;
+}
+
+static int
+_test_cfgfile_sample(struct rte_cfgfile *cfgfile)
+{
+	const char *value;
+	int ret;
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 2, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key1");
+	TEST_ASSERT(strcmp("value1", value) == 0,
+		    "key1 unexpected value: %s", value);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section2");
+	TEST_ASSERT(ret, "section2 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section2");
+	TEST_ASSERT(ret == 2, "section2 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key2");
+	TEST_ASSERT(strcmp("value2", value) == 0,
+		    "key2 unexpected value: %s", value);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key3");
+	TEST_ASSERT(strcmp("value3", value) == 0,
+		    "key3 unexpected value: %s", value);
+
+	return 0;
+}
+
+static int
+test_cfgfile_sample1(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/sample1.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_section_header(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_missing_section(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_empty_file(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 0, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile(void)
+{
+	if (test_cfgfile_setup())
+		return -1;
+
+	if (test_cfgfile_sample1())
+		return -1;
+
+	if (test_cfgfile_invalid_section_header())
+		return -1;
+
+	if (test_cfgfile_invalid_key_value_pair())
+		return -1;
+
+	if (test_cfgfile_missing_section())
+		return -1;
+
+	if (test_cfgfile_empty_file())
+		return -1;
+
+	if (test_cfgfile_cleanup())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(cfgfile_autotest, test_cfgfile);
diff --git a/test/test/test_cfgfiles/etc/empty.ini b/test/test/test_cfgfiles/etc/empty.ini
new file mode 100644
index 000000000..e69de29bb
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
new file mode 100644
index 000000000..8f203ce3e
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+invalid=
diff --git a/test/test/test_cfgfiles/etc/invalid_section.ini b/test/test/test_cfgfiles/etc/invalid_section.ini
new file mode 100644
index 000000000..95d680397
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/invalid_section.ini
@@ -0,0 +1,3 @@
+[invalid
+; this is section 1
+key1=value1
diff --git a/test/test/test_cfgfiles/etc/line_too_long.ini b/test/test/test_cfgfiles/etc/line_too_long.ini
new file mode 100644
index 000000000..1dce16483
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/line_too_long.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
diff --git a/test/test/test_cfgfiles/etc/missing_section.ini b/test/test/test_cfgfiles/etc/missing_section.ini
new file mode 100644
index 000000000..c78e131b4
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/missing_section.ini
@@ -0,0 +1,2 @@
+; no section
+key=value
diff --git a/test/test/test_cfgfiles/etc/sample1.ini b/test/test/test_cfgfiles/etc/sample1.ini
new file mode 100644
index 000000000..aef91c241
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample1.ini
@@ -0,0 +1,12 @@
+; this is a global comment
+
+[section1]
+; this is section 1
+key1=value1
+
+[section2]
+; this is section 2
+;key1=value1
+key2=value2
+key3=value3 ; this is key3
+ignore-missing-separator
-- 
2.12.1

[PATCH v5 2/6] cfgfile: add support for global properties section

[Allain Legacy]

The current implementation of the cfgfile library requires that all
key=value pairs be within [SECTION] definitions.  The ini file standard
allows for key=value pairs in an unnamed section.

   https://en.wikipedia.org/wiki/INI_file#Global_properties

This commit adds the capability of parsing key=value pairs from such an
unnamed section. The CFG_FLAG_GLOBAL_SECTION flag must be passed to the
rte_cfgfile_load() API to enable this functionality.  Any key=value pairs
found before the first section can be accessed in the section named
"GLOBAL".

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 16 ++++++++++++++++
 lib/librte_cfgfile/rte_cfgfile.h | 13 ++++++++++++-
 test/test/test_cfgfile.c         | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 829109a77..832fea829 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -107,6 +107,22 @@ rte_cfgfile_load(const char *filename, int flags)
 
 	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
 
+	if (flags & CFG_FLAG_GLOBAL_SECTION) {
+		curr_section = 0;
+		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
+		cfg->sections[curr_section] = malloc(
+			sizeof(*cfg->sections[0]) +
+			sizeof(cfg->sections[0]->entries[0]) *
+			allocated_entries);
+		if (cfg->sections[curr_section] == NULL) {
+			printf("Error - no memory for global section\n");
+			goto error1;
+		}
+
+		snprintf(cfg->sections[curr_section]->name,
+				 sizeof(cfg->sections[0]->name), "GLOBAL");
+	}
+
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *pos = NULL;
 		size_t len = strnlen(buffer, sizeof(buffer));
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index b40e6a135..2dadd75d4 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,13 +66,24 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/**@{ cfgfile load operation flags */
+enum {
+	/**
+	 * Indicates that the file supports key value entries before the first
+	 * defined section.  These entries can be accessed in the "GLOBAL"
+	 * section.
+	 */
+	CFG_FLAG_GLOBAL_SECTION = 1,
+};
+/**@} */
+
 /**
 * Open config file
 *
 * @param filename
 *   Config file name
 * @param flags
-*   Config file flags, Reserved for future use. Must be set to 0.
+*   Config file flags
 * @return
 *   Handle to configuration file on success, NULL otherwise
 */
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index faca89696..a7de3706c 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -163,6 +163,36 @@ test_cfgfile_missing_section(void)
 }
 
 static int
+test_cfgfile_global_properties(void)
+{
+	struct rte_cfgfile *cfgfile;
+	const char *value;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini",
+				   CFG_FLAG_GLOBAL_SECTION);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "GLOBAL");
+	TEST_ASSERT(ret, "global section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "GLOBAL");
+	TEST_ASSERT(ret == 1, "GLOBAL unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "GLOBAL", "key");
+	TEST_ASSERT(strcmp("value", value) == 0,
+		    "key unexpected value: %s", value);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_empty_file(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -198,6 +228,9 @@ test_cfgfile(void)
 	if (test_cfgfile_missing_section())
 		return -1;
 
+	if (test_cfgfile_global_properties())
+		return -1;
+
 	if (test_cfgfile_empty_file())
 		return -1;
 
-- 
2.12.1

[PATCH v5 3/6] cfgfile: add support for configurable comment character

[Allain Legacy]

The current cfgfile comment character is hardcoded to ';'.  This commit a
new API to allow the user to specify which comment character to use while
parsing the file.

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.

The approach of using a new API with an extensible parameters structure was
used rather than simply adding a new argument to the existing API to allow
for additional arguments to be introduced in the future.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c           | 61 +++++++++++++++++++++++++++++-
 lib/librte_cfgfile/rte_cfgfile.h           | 25 ++++++++++++
 lib/librte_cfgfile/rte_cfgfile_version.map |  7 ++++
 test/test/test_cfgfile.c                   | 47 +++++++++++++++++++++++
 test/test/test_cfgfiles/etc/sample2.ini    | 12 ++++++
 5 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 832fea829..63e34bbb0 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -35,6 +35,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
+#include <rte_common.h>
 #include <rte_string_fns.h>
 
 #include "rte_cfgfile.h"
@@ -58,6 +59,25 @@ struct rte_cfgfile {
  * for new entries do we add in */
 #define CFG_ALLOC_ENTRY_BATCH 16
 
+/**
+ * Default cfgfile load parameters.
+ */
+static const struct rte_cfgfile_parameters default_cfgfile_params = {
+	.comment_character = CFG_DEFAULT_COMMENT_CHARACTER,
+};
+
+/**
+ * Defines the list of acceptable comment characters supported by this
+ * library.
+ */
+static const char valid_comment_chars[] = {
+	'!',
+	'#',
+	'%',
+	';',
+	'@'
+};
+
 static unsigned
 _strip(char *str, unsigned len)
 {
@@ -85,9 +105,45 @@ _strip(char *str, unsigned len)
 	return newlen;
 }
 
+static int
+rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
+{
+	unsigned int valid_comment;
+	unsigned int i;
+
+	if (!params) {
+		printf("Error - missing cfgfile parameters\n");
+		return -EINVAL;
+	}
+
+	valid_comment = 0;
+	for (i = 0; i < RTE_DIM(valid_comment_chars); i++) {
+		if (params->comment_character == valid_comment_chars[i]) {
+			valid_comment = 1;
+			break;
+		}
+	}
+
+	if (valid_comment == 0)	{
+		printf("Error - invalid comment characters %c\n",
+		       params->comment_character);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 struct rte_cfgfile *
 rte_cfgfile_load(const char *filename, int flags)
 {
+	return rte_cfgfile_load_with_params(filename, flags,
+					    &default_cfgfile_params);
+}
+
+struct rte_cfgfile *
+rte_cfgfile_load_with_params(const char *filename, int flags,
+			     const struct rte_cfgfile_parameters *params)
+{
 	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
 	int allocated_entries = 0;
 	int curr_section = -1;
@@ -96,6 +152,9 @@ rte_cfgfile_load(const char *filename, int flags)
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
+	if (rte_cfgfile_check_params(params))
+		return NULL;
+
 	FILE *f = fopen(filename, "r");
 	if (f == NULL)
 		return NULL;
@@ -132,7 +191,7 @@ rte_cfgfile_load(const char *filename, int flags)
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, ';', sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, sizeof(buffer));
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 2dadd75d4..a267ed0b6 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/** Configuration file operation optional arguments */
+struct rte_cfgfile_parameters {
+	/** Config file comment character; one of '!', '#', '%', ';', '@' */
+	char comment_character;
+};
+
 /**@{ cfgfile load operation flags */
 enum {
 	/**
@@ -77,6 +83,9 @@ enum {
 };
 /**@} */
 
+/** Defines the default comment character used for parsing config files. */
+#define CFG_DEFAULT_COMMENT_CHARACTER ';'
+
 /**
 * Open config file
 *
@@ -90,6 +99,22 @@ enum {
 struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
 
 /**
+ * Open config file with specified optional parameters.
+ *
+ * @param filename
+ *   Config file name
+ * @param flags
+ *   Config file flags
+ * @param params
+ *   Additional configuration attributes.  Must be configured with desired
+ *   values prior to invoking this API.
+ * @return
+ *   Handle to configuration file on success, NULL otherwise
+ */
+struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
+	int flags, const struct rte_cfgfile_parameters *params);
+
+/**
 * Get number of sections in config file
 *
 * @param cfg
diff --git a/lib/librte_cfgfile/rte_cfgfile_version.map b/lib/librte_cfgfile/rte_cfgfile_version.map
index 3c2f0dbf4..5fe60f72e 100644
--- a/lib/librte_cfgfile/rte_cfgfile_version.map
+++ b/lib/librte_cfgfile/rte_cfgfile_version.map
@@ -20,3 +20,10 @@ DPDK_16.04 {
 	rte_cfgfile_section_entries_by_index;
 
 } DPDK_2.0;
+
+DPDK_17.05 {
+    global:
+
+    rte_cfgfile_load_with_params;
+
+} DPDK_16.04;
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index a7de3706c..8eabd711c 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -130,6 +130,30 @@ test_cfgfile_sample1(void)
 }
 
 static int
+test_cfgfile_sample2(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	/* override comment character */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '#';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_section_header(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -141,6 +165,23 @@ test_cfgfile_invalid_section_header(void)
 }
 
 static int
+test_cfgfile_invalid_comment(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+
+	/* override comment character with an invalid one */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '$';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_key_value_pair(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -219,9 +260,15 @@ test_cfgfile(void)
 	if (test_cfgfile_sample1())
 		return -1;
 
+	if (test_cfgfile_sample2())
+		return -1;
+
 	if (test_cfgfile_invalid_section_header())
 		return -1;
 
+	if (test_cfgfile_invalid_comment())
+		return -1;
+
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/sample2.ini b/test/test/test_cfgfiles/etc/sample2.ini
new file mode 100644
index 000000000..21075e976
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample2.ini
@@ -0,0 +1,12 @@
+# this is a global comment
+
+[section1]
+# this is section 1
+key1=value1
+
+[section2]
+# this is section 2
+#key1=value1
+key2=value2
+key3=value3 # this is key3
+ignore-missing-separator
-- 
2.12.1

[PATCH v5 4/6] cfgfile: use strnlen to constrain memchr search

[Allain Legacy]

The call to memchr() uses the absolute length of the string buffer instead
of the actual length of the string returned by fgets().  This causes the
search to go beyond the '\n' character and find ';' characters in random
garbage on the stack.  This then causes the 'len' variable to be updated
and the subsequent search for the '=' character to potentially find one
beyond the first newline character.

Since this bug relies on ';' and '=' characters appearing in random places
in the 'buffer' variable it is intermittently reproducible at best.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 63e34bbb0..e4a3885b7 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -191,7 +191,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, params->comment_character, sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, len);
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
-- 
2.12.1

[PATCH v5 5/6] cfgfile: increase local buffer size for max name and value

[Allain Legacy]

From: Joseph Richard <joseph.richard@windriver.com>

When parsing a ini file with a "key = value" line that has both "key" and
"value" sized to the maximum allowed length causes a parsing failure.  The
internal "buffer" variable should be sized at least as large as the maximum
for both fields.  This commit updates the local array to be sized to hold
the max name, max value, " = ", and the nul terminator.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index e4a3885b7..4ef7decb3 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -148,7 +148,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 	int allocated_entries = 0;
 	int curr_section = -1;
 	int curr_entry = -1;
-	char buffer[256] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
-- 
2.12.1

[PATCH v5 6/6] cfgfile: add support for empty value string

[Allain Legacy]

This commit adds support to the cfgfile library for parsing a key=value
line that has no value string specified (e.g., "key=").  This can be used
to override a configuration attribute that has a default value or default
list of values to set it back to an undefined value to disable
functionality.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c                | 23 ++++++++++++------
 lib/librte_cfgfile/rte_cfgfile.h                |  6 +++++
 test/test/test_cfgfile.c                        | 32 +++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty_key_value.ini |  2 +-
 4 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 4ef7decb3..b54a523d2 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -258,12 +258,21 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 
 			struct rte_cfgfile_section *sect =
 				cfg->sections[curr_section];
-			char *split[2];
-			if (rte_strsplit(buffer, sizeof(buffer), split, 2, '=')
-				!= 2) {
-				printf("Error at line %d - cannot split "
-					"string\n", lineno);
-				goto error1;
+			int n;
+			char *split[2] = {NULL};
+			n = rte_strsplit(buffer, sizeof(buffer), split, 2, '=');
+			if (flags & CFG_FLAG_EMPTY_VALUES) {
+				if ((n < 1) || (n > 2)) {
+					printf("Error at line %d - cannot split string, n=%d\n",
+					       lineno, n);
+					goto error1;
+				}
+			} else {
+				if (n != 2) {
+					printf("Error at line %d - cannot split string, n=%d\n",
+					       lineno, n);
+					goto error1;
+				}
 			}
 
 			curr_entry++;
@@ -293,7 +302,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 			snprintf(entry->name, sizeof(entry->name), "%s",
 				split[0]);
 			snprintf(entry->value, sizeof(entry->value), "%s",
-				split[1]);
+				 split[1] ? split[1] : "");
 			_strip(entry->name, strnlen(entry->name,
 				sizeof(entry->name)));
 			_strip(entry->value, strnlen(entry->value,
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index a267ed0b6..fa10d4089 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -80,6 +80,12 @@ enum {
 	 * section.
 	 */
 	CFG_FLAG_GLOBAL_SECTION = 1,
+
+	/**
+	 * Indicates that file supports key value entries where the value can
+	 * be zero length (e.g., "key=").
+	 */
+	CFG_FLAG_EMPTY_VALUES = 2,
 };
 /**@} */
 
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index 8eabd711c..4cc9b14dd 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -193,6 +193,35 @@ test_cfgfile_invalid_key_value_pair(void)
 }
 
 static int
+test_cfgfile_empty_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+	const char *value;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini",
+				   CFG_FLAG_EMPTY_VALUES);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse empty_key_value.ini");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key");
+	TEST_ASSERT(strlen(value) == 0, "key unexpected value: %s", value);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_missing_section(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -272,6 +301,9 @@ test_cfgfile(void)
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
+	if (test_cfgfile_empty_key_value_pair())
+		return -1;
+
 	if (test_cfgfile_missing_section())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
index 8f203ce3e..53284467b 100644
--- a/test/test/test_cfgfiles/etc/empty_key_value.ini
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -1,3 +1,3 @@
 [section1]
 ; this is section 1
-invalid=
+key=
-- 
2.12.1

Re: [PATCH v5 0/6] librte_cfgfile enhancements

[Thomas Monjalon]

2017-03-31 09:51, Allain Legacy:
> This patchset includes some minor enhancements that we have developped for
> our DPDK application.  We would like to contribute them upstream to help
> ease adoption of the DPDK by anyone looking for this type of
> functionality.  The commit logs on each patch should be self-sufficient in
> explaining the intent and purpose.

Adding this missing line in MAINTAINERS:

 Configuration file
 M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
 F: lib/librte_cfgfile/
+F: test/test/test_cfgfiles/
 F: test/test/test_cfgfile*

And applied, thanks

[PATCH v3 1/6] test: basic unit tests for cfgfile

[Allain Legacy]

This commit adds the basic infrastructure for the cfgfile library unit
tests.  It includes success path tests for the most commonly used APIs.
More unit tests will be added later.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 MAINTAINERS                                     |   1 +
 test/test/Makefile                              |   2 +
 test/test/test_cfgfile.c                        | 210 ++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/empty.ini           |   0
 test/test/test_cfgfiles/etc/empty_key_value.ini |   3 +
 test/test/test_cfgfiles/etc/invalid_section.ini |   3 +
 test/test/test_cfgfiles/etc/line_too_long.ini   |   3 +
 test/test/test_cfgfiles/etc/missing_section.ini |   2 +
 test/test/test_cfgfiles/etc/sample1.ini         |  12 ++
 9 files changed, 236 insertions(+)
 create mode 100644 test/test/test_cfgfile.c
 create mode 100644 test/test/test_cfgfiles/etc/empty.ini
 create mode 100644 test/test/test_cfgfiles/etc/empty_key_value.ini
 create mode 100644 test/test/test_cfgfiles/etc/invalid_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/line_too_long.ini
 create mode 100644 test/test/test_cfgfiles/etc/missing_section.ini
 create mode 100644 test/test/test_cfgfiles/etc/sample1.ini

diff --git a/MAINTAINERS b/MAINTAINERS
index 0b1524d3c..8eeb907a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -588,6 +588,7 @@ Other libraries
 Configuration file
 M: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
 F: lib/librte_cfgfile/
+F: test/test/test_cfgfile*
 
 Interactive command line
 M: Olivier Matz <olivier.matz@6wind.com>
diff --git a/test/test/Makefile b/test/test/Makefile
index 79f0c6179..60c1761fa 100644
--- a/test/test/Makefile
+++ b/test/test/Makefile
@@ -85,6 +85,8 @@ $(eval $(call linked_resource,test_resource_c,test_resource.res))
 $(eval $(call linked_tar_resource,test_resource_tar,test_resource.c))
 SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_pci.c
 $(eval $(call linked_tar_resource,test_pci_sysfs,test_pci_sysfs))
+SRCS-$(CONFIG_RTE_APP_TEST_RESOURCE_TAR) += test_cfgfile.c
+$(eval $(call linked_tar_resource,test_cfgfiles,test_cfgfiles))
 SRCS-y += test_prefetch.c
 SRCS-y += test_byteorder.c
 SRCS-y += test_per_lcore.c
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
new file mode 100644
index 000000000..faca89696
--- /dev/null
+++ b/test/test/test_cfgfile.c
@@ -0,0 +1,210 @@
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2017 Wind River Systems Inc. All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+#include <sys/queue.h>
+
+#include <rte_cfgfile.h>
+
+#include "test.h"
+#include "resource.h"
+
+
+#define CFG_FILES_ETC "test_cfgfiles/etc"
+
+REGISTER_LINKED_RESOURCE(test_cfgfiles);
+
+static int
+test_cfgfile_setup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_untar(r);
+	TEST_ASSERT_SUCCESS(ret, "failed to untar %s", r->name);
+
+	return 0;
+}
+
+static int
+test_cfgfile_cleanup(void)
+{
+	const struct resource *r;
+	int ret;
+
+	r = resource_find("test_cfgfiles");
+	TEST_ASSERT_NOT_NULL(r, "missing resource test_cfgfiles");
+
+	ret = resource_rm_by_tar(r);
+	TEST_ASSERT_SUCCESS(ret, "Failed to delete resource %s", r->name);
+
+	return 0;
+}
+
+static int
+_test_cfgfile_sample(struct rte_cfgfile *cfgfile)
+{
+	const char *value;
+	int ret;
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 2, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section1");
+	TEST_ASSERT(ret, "section1 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
+	TEST_ASSERT(ret == 1, "section1 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section1", "key1");
+	TEST_ASSERT(strcmp("value1", value) == 0,
+		    "key1 unexpected value: %s", value);
+
+	ret = rte_cfgfile_has_section(cfgfile, "section2");
+	TEST_ASSERT(ret, "section2 section missing");
+
+	ret = rte_cfgfile_section_num_entries(cfgfile, "section2");
+	TEST_ASSERT(ret == 2, "section2 unexpected number of entries: %d", ret);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key2");
+	TEST_ASSERT(strcmp("value2", value) == 0,
+		    "key2 unexpected value: %s", value);
+
+	value = rte_cfgfile_get_entry(cfgfile, "section2", "key3");
+	TEST_ASSERT(strcmp("value3", value) == 0,
+		    "key3 unexpected value: %s", value);
+
+	return 0;
+}
+
+static int
+test_cfgfile_sample1(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/sample1.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_section_header(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/invalid_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_invalid_key_value_pair(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_missing_section(void)
+{
+	struct rte_cfgfile *cfgfile;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini", 0);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
+test_cfgfile_empty_file(void)
+{
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty.ini", 0);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to load config file");
+
+	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
+	TEST_ASSERT(ret == 0, "Unexpected number of sections: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
+test_cfgfile(void)
+{
+	if (test_cfgfile_setup())
+		return -1;
+
+	if (test_cfgfile_sample1())
+		return -1;
+
+	if (test_cfgfile_invalid_section_header())
+		return -1;
+
+	if (test_cfgfile_invalid_key_value_pair())
+		return -1;
+
+	if (test_cfgfile_missing_section())
+		return -1;
+
+	if (test_cfgfile_empty_file())
+		return -1;
+
+	if (test_cfgfile_cleanup())
+		return -1;
+
+	return 0;
+}
+
+REGISTER_TEST_COMMAND(cfgfile_autotest, test_cfgfile);
diff --git a/test/test/test_cfgfiles/etc/empty.ini b/test/test/test_cfgfiles/etc/empty.ini
new file mode 100644
index 000000000..e69de29bb
diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini b/test/test/test_cfgfiles/etc/empty_key_value.ini
new file mode 100644
index 000000000..8f203ce3e
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+invalid=
diff --git a/test/test/test_cfgfiles/etc/invalid_section.ini b/test/test/test_cfgfiles/etc/invalid_section.ini
new file mode 100644
index 000000000..95d680397
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/invalid_section.ini
@@ -0,0 +1,3 @@
+[invalid
+; this is section 1
+key1=value1
diff --git a/test/test/test_cfgfiles/etc/line_too_long.ini b/test/test/test_cfgfiles/etc/line_too_long.ini
new file mode 100644
index 000000000..1dce16483
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/line_too_long.ini
@@ -0,0 +1,3 @@
+[section1]
+; this is section 1
+012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789
diff --git a/test/test/test_cfgfiles/etc/missing_section.ini b/test/test/test_cfgfiles/etc/missing_section.ini
new file mode 100644
index 000000000..c78e131b4
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/missing_section.ini
@@ -0,0 +1,2 @@
+; no section
+key=value
diff --git a/test/test/test_cfgfiles/etc/sample1.ini b/test/test/test_cfgfiles/etc/sample1.ini
new file mode 100644
index 000000000..aef91c241
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample1.ini
@@ -0,0 +1,12 @@
+; this is a global comment
+
+[section1]
+; this is section 1
+key1=value1
+
+[section2]
+; this is section 2
+;key1=value1
+key2=value2
+key3=value3 ; this is key3
+ignore-missing-separator
-- 
2.12.1

[PATCH v3 3/6] cfgfile: add support for configurable comment character

[Allain Legacy]

The current cfgfile comment character is hardcoded to ';'.  This commit a
new API to allow the user to specify which comment character to use while
parsing the file.

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.

The approach of using a new API with an extensible parameters structure was
used rather than simply adding a new argument to the existing API to allow
for additional arguments to be introduced in the future.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
---
 lib/librte_cfgfile/rte_cfgfile.c        | 61 ++++++++++++++++++++++++++++++++-
 lib/librte_cfgfile/rte_cfgfile.h        | 29 ++++++++++++++++
 test/test/test_cfgfile.c                | 47 +++++++++++++++++++++++++
 test/test/test_cfgfiles/etc/sample2.ini | 12 +++++++
 4 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 test/test/test_cfgfiles/etc/sample2.ini

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 832fea829..63e34bbb0 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -35,6 +35,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
+#include <rte_common.h>
 #include <rte_string_fns.h>
 
 #include "rte_cfgfile.h"
@@ -58,6 +59,25 @@ struct rte_cfgfile {
  * for new entries do we add in */
 #define CFG_ALLOC_ENTRY_BATCH 16
 
+/**
+ * Default cfgfile load parameters.
+ */
+static const struct rte_cfgfile_parameters default_cfgfile_params = {
+	.comment_character = CFG_DEFAULT_COMMENT_CHARACTER,
+};
+
+/**
+ * Defines the list of acceptable comment characters supported by this
+ * library.
+ */
+static const char valid_comment_chars[] = {
+	'!',
+	'#',
+	'%',
+	';',
+	'@'
+};
+
 static unsigned
 _strip(char *str, unsigned len)
 {
@@ -85,9 +105,45 @@ _strip(char *str, unsigned len)
 	return newlen;
 }
 
+static int
+rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
+{
+	unsigned int valid_comment;
+	unsigned int i;
+
+	if (!params) {
+		printf("Error - missing cfgfile parameters\n");
+		return -EINVAL;
+	}
+
+	valid_comment = 0;
+	for (i = 0; i < RTE_DIM(valid_comment_chars); i++) {
+		if (params->comment_character == valid_comment_chars[i]) {
+			valid_comment = 1;
+			break;
+		}
+	}
+
+	if (valid_comment == 0)	{
+		printf("Error - invalid comment characters %c\n",
+		       params->comment_character);
+		return -ENOTSUP;
+	}
+
+	return 0;
+}
+
 struct rte_cfgfile *
 rte_cfgfile_load(const char *filename, int flags)
 {
+	return rte_cfgfile_load_with_params(filename, flags,
+					    &default_cfgfile_params);
+}
+
+struct rte_cfgfile *
+rte_cfgfile_load_with_params(const char *filename, int flags,
+			     const struct rte_cfgfile_parameters *params)
+{
 	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
 	int allocated_entries = 0;
 	int curr_section = -1;
@@ -96,6 +152,9 @@ rte_cfgfile_load(const char *filename, int flags)
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
+	if (rte_cfgfile_check_params(params))
+		return NULL;
+
 	FILE *f = fopen(filename, "r");
 	if (f == NULL)
 		return NULL;
@@ -132,7 +191,7 @@ rte_cfgfile_load(const char *filename, int flags)
 					"Check if line too long\n", lineno);
 			goto error1;
 		}
-		pos = memchr(buffer, ';', sizeof(buffer));
+		pos = memchr(buffer, params->comment_character, sizeof(buffer));
 		if (pos != NULL) {
 			*pos = '\0';
 			len = pos -  buffer;
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 0e805c268..a7cd3944e 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
 	char value[CFG_VALUE_LEN]; /**< Value */
 };
 
+/** Configuration file operation optional arguments */
+struct rte_cfgfile_parameters {
+	char comment_character;
+	/**< Config file comment character; one of '!', '#', '%', ';', '@' */
+};
+
 /**@{ cfgfile load operation flags */
 /**
  * Indicates that the file supports key value entries before the first defined
@@ -74,6 +80,9 @@ struct rte_cfgfile_entry {
 #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
 /**@} */
 
+/** Defines the default comment character used for parsing config files. */
+#define CFG_DEFAULT_COMMENT_CHARACTER ';'
+
 /**
 * Open config file
 *
@@ -87,6 +96,26 @@ struct rte_cfgfile_entry {
 struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
 
 /**
+ * Open config file with specified optional parameters.  Use @see
+ * rte_cfgfile_init_parameters to setup the default parameters.
+ *
+ * @param filename
+ *   Config file name
+ * @param flags
+ *   Config file flags
+ * @param params
+ *   Additional configuration attributes.  Must be configured with default
+ *   values prior to invoking this API.
+ * @return
+ *   Handle to configuration file on success, NULL otherwise
+ * @param
+ *
+ */
+struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
+	int flags, const struct rte_cfgfile_parameters *params);
+
+
+/**
 * Get number of sections in config file
 *
 * @param cfg
diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
index ad293cc89..0478de59e 100644
--- a/test/test/test_cfgfile.c
+++ b/test/test/test_cfgfile.c
@@ -130,6 +130,30 @@ test_cfgfile_sample1(void)
 }
 
 static int
+test_cfgfile_sample2(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+	int ret;
+
+	/* override comment character */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '#';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
+
+	ret = _test_cfgfile_sample(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
+
+	ret = rte_cfgfile_close(cfgfile);
+	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_section_header(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -141,6 +165,23 @@ test_cfgfile_invalid_section_header(void)
 }
 
 static int
+test_cfgfile_invalid_comment(void)
+{
+	struct rte_cfgfile_parameters params;
+	struct rte_cfgfile *cfgfile;
+
+	/* override comment character with an invalid one */
+	memset(&params, 0, sizeof(params));
+	params.comment_character = '$';
+
+	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC "/sample2.ini", 0,
+					       &params);
+	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
+
+	return 0;
+}
+
+static int
 test_cfgfile_invalid_key_value_pair(void)
 {
 	struct rte_cfgfile *cfgfile;
@@ -219,9 +260,15 @@ test_cfgfile(void)
 	if (test_cfgfile_sample1())
 		return -1;
 
+	if (test_cfgfile_sample2())
+		return -1;
+
 	if (test_cfgfile_invalid_section_header())
 		return -1;
 
+	if (test_cfgfile_invalid_comment())
+		return -1;
+
 	if (test_cfgfile_invalid_key_value_pair())
 		return -1;
 
diff --git a/test/test/test_cfgfiles/etc/sample2.ini b/test/test/test_cfgfiles/etc/sample2.ini
new file mode 100644
index 000000000..21075e976
--- /dev/null
+++ b/test/test/test_cfgfiles/etc/sample2.ini
@@ -0,0 +1,12 @@
+# this is a global comment
+
+[section1]
+# this is section 1
+key1=value1
+
+[section2]
+# this is section 2
+#key1=value1
+key2=value2
+key3=value3 # this is key3
+ignore-missing-separator
-- 
2.12.1

Re: [PATCH v3 3/6] cfgfile: add support for configurable comment character

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Wednesday, March 29, 2017 1:48 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com;
> thomas.monjalon@6wind.com
> Subject: [PATCH v3 3/6] cfgfile: add support for configurable comment
> character
> 
> The current cfgfile comment character is hardcoded to ';'.  This commit a
> new API to allow the user to specify which comment character to use while
> parsing the file.
> 
> 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.
> 
> The approach of using a new API with an extensible parameters structure
> was
> used rather than simply adding a new argument to the existing API to allow
> for additional arguments to be introduced in the future.
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c        | 61
> ++++++++++++++++++++++++++++++++-
>  lib/librte_cfgfile/rte_cfgfile.h        | 29 ++++++++++++++++
>  test/test/test_cfgfile.c                | 47 +++++++++++++++++++++++++
>  test/test/test_cfgfiles/etc/sample2.ini | 12 +++++++
>  4 files changed, 148 insertions(+), 1 deletion(-)
>  create mode 100644 test/test/test_cfgfiles/etc/sample2.ini
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 832fea829..63e34bbb0 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -35,6 +35,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <ctype.h>
> +#include <rte_common.h>
>  #include <rte_string_fns.h>
> 
>  #include "rte_cfgfile.h"
> @@ -58,6 +59,25 @@ struct rte_cfgfile {
>   * for new entries do we add in */
>  #define CFG_ALLOC_ENTRY_BATCH 16
> 
> +/**
> + * Default cfgfile load parameters.
> + */
> +static const struct rte_cfgfile_parameters default_cfgfile_params = {
> +	.comment_character = CFG_DEFAULT_COMMENT_CHARACTER,
> +};
> +
> +/**
> + * Defines the list of acceptable comment characters supported by this
> + * library.
> + */
> +static const char valid_comment_chars[] = {
> +	'!',
> +	'#',
> +	'%',
> +	';',
> +	'@'
> +};
> +
>  static unsigned
>  _strip(char *str, unsigned len)
>  {
> @@ -85,9 +105,45 @@ _strip(char *str, unsigned len)
>  	return newlen;
>  }
> 
> +static int
> +rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
> +{
> +	unsigned int valid_comment;
> +	unsigned int i;
> +
> +	if (!params) {
> +		printf("Error - missing cfgfile parameters\n");
> +		return -EINVAL;
> +	}
> +
> +	valid_comment = 0;
> +	for (i = 0; i < RTE_DIM(valid_comment_chars); i++) {
> +		if (params->comment_character == valid_comment_chars[i])
> {
> +			valid_comment = 1;
> +			break;
> +		}
> +	}
> +
> +	if (valid_comment == 0)	{
> +		printf("Error - invalid comment characters %c\n",
> +		       params->comment_character);
> +		return -ENOTSUP;
> +	}
> +
> +	return 0;
> +}
> +
>  struct rte_cfgfile *
>  rte_cfgfile_load(const char *filename, int flags)
>  {
> +	return rte_cfgfile_load_with_params(filename, flags,
> +					    &default_cfgfile_params);
> +}
> +
> +struct rte_cfgfile *
> +rte_cfgfile_load_with_params(const char *filename, int flags,
> +			     const struct rte_cfgfile_parameters *params)
> +{
>  	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
>  	int allocated_entries = 0;
>  	int curr_section = -1;
> @@ -96,6 +152,9 @@ rte_cfgfile_load(const char *filename, int flags)
>  	int lineno = 0;
>  	struct rte_cfgfile *cfg = NULL;
> 
> +	if (rte_cfgfile_check_params(params))
> +		return NULL;
> +
>  	FILE *f = fopen(filename, "r");
>  	if (f == NULL)
>  		return NULL;
> @@ -132,7 +191,7 @@ rte_cfgfile_load(const char *filename, int flags)
>  					"Check if line too long\n", lineno);
>  			goto error1;
>  		}
> -		pos = memchr(buffer, ';', sizeof(buffer));
> +		pos = memchr(buffer, params->comment_character,
> sizeof(buffer));
>  		if (pos != NULL) {
>  			*pos = '\0';
>  			len = pos -  buffer;
> diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
> index 0e805c268..a7cd3944e 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.h
> +++ b/lib/librte_cfgfile/rte_cfgfile.h
> @@ -66,6 +66,12 @@ struct rte_cfgfile_entry {
>  	char value[CFG_VALUE_LEN]; /**< Value */
>  };
> 
> +/** Configuration file operation optional arguments */
> +struct rte_cfgfile_parameters {
> +	char comment_character;
> +	/**< Config file comment character; one of '!', '#', '%', ';', '@' */

I think this comment should be placed above the structure field definition (preferable) or on the same line, but not below.

> +};
> +
>  /**@{ cfgfile load operation flags */
>  /**
>   * Indicates that the file supports key value entries before the first defined
> @@ -74,6 +80,9 @@ struct rte_cfgfile_entry {
>  #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
>  /**@} */
> 
> +/** Defines the default comment character used for parsing config files. */
> +#define CFG_DEFAULT_COMMENT_CHARACTER ';'
> +
>  /**
>  * Open config file
>  *
> @@ -87,6 +96,26 @@ struct rte_cfgfile_entry {
>  struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags);
> 
>  /**
> + * Open config file with specified optional parameters.  Use @see
> + * rte_cfgfile_init_parameters to setup the default parameters.
> + *

Please update this comment, as there is no rte_cfgfile_init() function anymore.

> + * @param filename
> + *   Config file name
> + * @param flags
> + *   Config file flags
> + * @param params
> + *   Additional configuration attributes.  Must be configured with default
> + *   values prior to invoking this API.

Same here.

> + * @return
> + *   Handle to configuration file on success, NULL otherwise
> + * @param
> + *
> + */
> +struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
> +	int flags, const struct rte_cfgfile_parameters *params);
> +
> +
> +/**
>  * Get number of sections in config file
>  *
>  * @param cfg
> diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
> index ad293cc89..0478de59e 100644
> --- a/test/test/test_cfgfile.c
> +++ b/test/test/test_cfgfile.c
> @@ -130,6 +130,30 @@ test_cfgfile_sample1(void)
>  }
> 
>  static int
> +test_cfgfile_sample2(void)
> +{
> +	struct rte_cfgfile_parameters params;
> +	struct rte_cfgfile *cfgfile;
> +	int ret;
> +
> +	/* override comment character */
> +	memset(&params, 0, sizeof(params));
> +	params.comment_character = '#';
> +
> +	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC
> "/sample2.ini", 0,
> +					       &params);
> +	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini");
> +
> +	ret = _test_cfgfile_sample(cfgfile);
> +	TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret);
> +
> +	ret = rte_cfgfile_close(cfgfile);
> +	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
> +
> +	return 0;
> +}
> +
> +static int
>  test_cfgfile_invalid_section_header(void)
>  {
>  	struct rte_cfgfile *cfgfile;
> @@ -141,6 +165,23 @@ test_cfgfile_invalid_section_header(void)
>  }
> 
>  static int
> +test_cfgfile_invalid_comment(void)
> +{
> +	struct rte_cfgfile_parameters params;
> +	struct rte_cfgfile *cfgfile;
> +
> +	/* override comment character with an invalid one */
> +	memset(&params, 0, sizeof(params));
> +	params.comment_character = '$';
> +
> +	cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC
> "/sample2.ini", 0,
> +					       &params);
> +	TEST_ASSERT_NULL(cfgfile, "Expected failured did not occur");
> +
> +	return 0;
> +}
> +
> +static int
>  test_cfgfile_invalid_key_value_pair(void)
>  {
>  	struct rte_cfgfile *cfgfile;
> @@ -219,9 +260,15 @@ test_cfgfile(void)
>  	if (test_cfgfile_sample1())
>  		return -1;
> 
> +	if (test_cfgfile_sample2())
> +		return -1;
> +
>  	if (test_cfgfile_invalid_section_header())
>  		return -1;
> 
> +	if (test_cfgfile_invalid_comment())
> +		return -1;
> +
>  	if (test_cfgfile_invalid_key_value_pair())
>  		return -1;
> 
> diff --git a/test/test/test_cfgfiles/etc/sample2.ini
> b/test/test/test_cfgfiles/etc/sample2.ini
> new file mode 100644
> index 000000000..21075e976
> --- /dev/null
> +++ b/test/test/test_cfgfiles/etc/sample2.ini
> @@ -0,0 +1,12 @@
> +# this is a global comment
> +
> +[section1]
> +# this is section 1
> +key1=value1
> +
> +[section2]
> +# this is section 2
> +#key1=value1
> +key2=value2
> +key3=value3 # this is key3
> +ignore-missing-separator
> --
> 2.12.1

Re: [PATCH v3 3/6] cfgfile: add support for configurable comment character

[Legacy, Allain]

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> Sent: Wednesday, March 29, 2017 5:23 AM
> >
> > +/** Configuration file operation optional arguments */ struct
> > +rte_cfgfile_parameters {
> > +	char comment_character;
> > +	/**< Config file comment character; one of '!', '#', '%', ';', '@'
> > +*/
> 
> I think this comment should be placed above the structure field definition
> (preferable) or on the same line, but not below.

Ok.  I will relocate it in v4 to be above.  The /**< */ construct allows for putting comments on the following line. 

> >  /**
> > + * Open config file with specified optional parameters.  Use @see
> > + * rte_cfgfile_init_parameters to setup the default parameters.
> > + *
> 
> Please update this comment, as there is no rte_cfgfile_init() function
> anymore.


Ok.  Will do.

[PATCH v3 5/6] cfgfile: increase local buffer size for max name and value

[Allain Legacy]

From: Joseph Richard <joseph.richard@windriver.com>

When parsing a ini file with a "key = value" line that has both "key" and
"value" sized to the maximum allowed length causes a parsing failure.  The
internal "buffer" variable should be sized at least as large as the maximum
for both fields.  This commit updates the local array to be sized to hold
the max name, max value, " = ", and the nul terminator.

Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index e4a3885b7..4ef7decb3 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -148,7 +148,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 	int allocated_entries = 0;
 	int curr_section = -1;
 	int curr_entry = -1;
-	char buffer[256] = {0};
+	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
 	int lineno = 0;
 	struct rte_cfgfile *cfg = NULL;
 
-- 
2.12.1

Re: [PATCH v3 6/6] cfgfile: add support for empty value string

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Wednesday, March 29, 2017 1:48 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com;
> thomas.monjalon@6wind.com
> Subject: [PATCH v3 6/6] cfgfile: add support for empty value string
> 
> This commit adds support to the cfgfile library for parsing a key=value
> line that has no value string specified (e.g., "key=").  This can be used
> to override a configuration attribute that has a default value or default
> list of values to set it back to an undefined value to disable
> functionality.
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c                | 23 ++++++++++++------
>  lib/librte_cfgfile/rte_cfgfile.h                |  6 +++++
>  test/test/test_cfgfile.c                        | 32 +++++++++++++++++++++++++
>  test/test/test_cfgfiles/etc/empty_key_value.ini |  2 +-
>  4 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 4ef7decb3..b54a523d2 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -258,12 +258,21 @@ rte_cfgfile_load_with_params(const char
> *filename, int flags,
> 
>  			struct rte_cfgfile_section *sect =
>  				cfg->sections[curr_section];
> -			char *split[2];
> -			if (rte_strsplit(buffer, sizeof(buffer), split, 2, '=')
> -				!= 2) {
> -				printf("Error at line %d - cannot split "
> -					"string\n", lineno);
> -				goto error1;
> +			int n;
> +			char *split[2] = {NULL};
> +			n = rte_strsplit(buffer, sizeof(buffer), split, 2, '=');
> +			if (flags & CFG_FLAG_EMPTY_VALUES) {
> +				if ((n < 1) || (n > 2)) {
> +					printf("Error at line %d - cannot split
> string, n=%d\n",
> +					       lineno, n);
> +					goto error1;
> +				}
> +			} else {
> +				if (n != 2) {
> +					printf("Error at line %d - cannot split
> string, n=%d\n",
> +					       lineno, n);
> +					goto error1;
> +				}
>  			}
> 
>  			curr_entry++;
> @@ -293,7 +302,7 @@ rte_cfgfile_load_with_params(const char *filename,
> int flags,
>  			snprintf(entry->name, sizeof(entry->name), "%s",
>  				split[0]);
>  			snprintf(entry->value, sizeof(entry->value), "%s",
> -				split[1]);
> +				 split[1] ? split[1] : "");
>  			_strip(entry->name, strnlen(entry->name,
>  				sizeof(entry->name)));
>  			_strip(entry->value, strnlen(entry->value,
> diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
> index a7cd3944e..b06f55d56 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.h
> +++ b/lib/librte_cfgfile/rte_cfgfile.h
> @@ -78,6 +78,12 @@ struct rte_cfgfile_parameters {
>   * section.  These entries can be accessed in the "GLOBAL" section.
>   */
>  #define CFG_FLAG_GLOBAL_SECTION (1 << 0)
> +
> +/**
> + * Indicates that file supports key value entries where the value can be zero
> + * length (e.g., "key=").
> + */
> +#define CFG_FLAG_EMPTY_VALUES (1 << 1)
>  /**@} */
> 

I suggest we group all these flags into an (unnamed?) enum in rte_cfgfile.h rather than macros.

>  /** Defines the default comment character used for parsing config files. */
> diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
> index 0478de59e..4559f8688 100644
> --- a/test/test/test_cfgfile.c
> +++ b/test/test/test_cfgfile.c
> @@ -193,6 +193,35 @@ test_cfgfile_invalid_key_value_pair(void)
>  }
> 
>  static int
> +test_cfgfile_empty_key_value_pair(void)
> +{
> +	struct rte_cfgfile *cfgfile;
> +	const char *value;
> +	int ret;
> +
> +	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/empty_key_value.ini",
> +				   CFG_FLAG_EMPTY_VALUES);
> +	TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse
> empty_key_value.ini");
> +
> +	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
> +	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
> +
> +	ret = rte_cfgfile_has_section(cfgfile, "section1");
> +	TEST_ASSERT(ret, "section1 missing");
> +
> +	ret = rte_cfgfile_section_num_entries(cfgfile, "section1");
> +	TEST_ASSERT(ret == 1, "section1 unexpected number of entries:
> %d", ret);
> +
> +	value = rte_cfgfile_get_entry(cfgfile, "section1", "key");
> +	TEST_ASSERT(strlen(value) == 0, "key unexpected value: %s", value);
> +
> +	ret = rte_cfgfile_close(cfgfile);
> +	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
> +
> +	return 0;
> +}
> +

Can we please add a test to validate that error is triggered for empty key values in the case the CFG_FLAG_EMPTY_VALUES is NOT provided to rte_cfgfile_load()?

> +static int
>  test_cfgfile_missing_section(void)
>  {
>  	struct rte_cfgfile *cfgfile;
> @@ -272,6 +301,9 @@ test_cfgfile(void)
>  	if (test_cfgfile_invalid_key_value_pair())
>  		return -1;
> 
> +	if (test_cfgfile_empty_key_value_pair())
> +		return -1;
> +
>  	if (test_cfgfile_missing_section())
>  		return -1;
> 
> diff --git a/test/test/test_cfgfiles/etc/empty_key_value.ini
> b/test/test/test_cfgfiles/etc/empty_key_value.ini
> index 8f203ce3e..53284467b 100644
> --- a/test/test/test_cfgfiles/etc/empty_key_value.ini
> +++ b/test/test/test_cfgfiles/etc/empty_key_value.ini
> @@ -1,3 +1,3 @@
>  [section1]
>  ; this is section 1
> -invalid=
> +key=
> --
> 2.12.1

Re: [PATCH v3 6/6] cfgfile: add support for empty value string

[Legacy, Allain]

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> Sent: Wednesday, March 29, 2017 5:31 AM

<...>
> > +#define CFG_FLAG_EMPTY_VALUES (1 << 1)
> >  /**@} */
> >
> 
> I suggest we group all these flags into an (unnamed?) enum in rte_cfgfile.h
> rather than macros.
Ok.  Will do.

> 
> Can we please add a test to validate that error is triggered for empty key
> values in the case the CFG_FLAG_EMPTY_VALUES is NOT provided to
> rte_cfgfile_load()?

Already included.  See test_cfgfile_invalid_key_value_pair().

Re: [PATCH v3 2/6] cfgfile: add support for global properties section

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Allain Legacy [mailto:allain.legacy@windriver.com]
> Sent: Wednesday, March 29, 2017 1:48 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com;
> thomas.monjalon@6wind.com
> Subject: [PATCH v3 2/6] cfgfile: add support for global properties section
> 
> The current implementation of the cfgfile library requires that all
> key=value pairs be within [SECTION] definitions.  The ini file standard
> allows for key=value pairs in an unnamed section.
> 
>    https://en.wikipedia.org/wiki/INI_file#Global_properties
> 
> This commit adds the capability of parsing key=value pairs from such an
> unnamed section. The CFG_FLAG_GLOBAL_SECTION flag must be passed to
> the
> rte_cfgfile_load() API to enable this functionality.  Any key=value pairs
> found before the first section can be accessed in the section named
> "GLOBAL".
> 
> Signed-off-by: Allain Legacy <allain.legacy@windriver.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 16 ++++++++++++++++
>  lib/librte_cfgfile/rte_cfgfile.h | 10 +++++++++-
>  test/test/test_cfgfile.c         | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 829109a77..832fea829 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -107,6 +107,22 @@ rte_cfgfile_load(const char *filename, int flags)
> 
>  	memset(cfg->sections, 0, sizeof(cfg->sections[0]) *
> allocated_sections);
> 
> +	if (flags & CFG_FLAG_GLOBAL_SECTION) {
> +		curr_section = 0;
> +		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
> +		cfg->sections[curr_section] = malloc(
> +			sizeof(*cfg->sections[0]) +
> +			sizeof(cfg->sections[0]->entries[0]) *
> +			allocated_entries);
> +		if (cfg->sections[curr_section] == NULL) {
> +			printf("Error - no memory for global section\n");
> +			goto error1;
> +		}
> +
> +		snprintf(cfg->sections[curr_section]->name,
> +				 sizeof(cfg->sections[0]->name), "GLOBAL");
> +	}
> +
>  	while (fgets(buffer, sizeof(buffer), f) != NULL) {
>  		char *pos = NULL;
>  		size_t len = strnlen(buffer, sizeof(buffer));
> diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
> index b40e6a135..0e805c268 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.h
> +++ b/lib/librte_cfgfile/rte_cfgfile.h
> @@ -66,13 +66,21 @@ struct rte_cfgfile_entry {
>  	char value[CFG_VALUE_LEN]; /**< Value */
>  };
> 
> +/**@{ cfgfile load operation flags */
> +/**
> + * Indicates that the file supports key value entries before the first defined
> + * section.  These entries can be accessed in the "GLOBAL" section.
> + */
> +#define CFG_FLAG_GLOBAL_SECTION (1 << 0)
> +/**@} */
> +
>  /**
>  * Open config file
>  *
>  * @param filename
>  *   Config file name
>  * @param flags
> -*   Config file flags, Reserved for future use. Must be set to 0.
> +*   Config file flags
>  * @return
>  *   Handle to configuration file on success, NULL otherwise
>  */
> diff --git a/test/test/test_cfgfile.c b/test/test/test_cfgfile.c
> index faca89696..ad293cc89 100644
> --- a/test/test/test_cfgfile.c
> +++ b/test/test/test_cfgfile.c
> @@ -163,6 +163,36 @@ test_cfgfile_missing_section(void)
>  }
> 
>  static int
> +test_cfgfile_global_properties(void)
> +{
> +	struct rte_cfgfile *cfgfile;
> +	const char *value;
> +	int ret;
> +
> +	cfgfile = rte_cfgfile_load(CFG_FILES_ETC "/missing_section.ini",
> +				   CFG_FLAG_GLOBAL_SECTION);
> +	TEST_ASSERT_NOT_NULL(cfgfile, "Expected failured did not occur");
> +
> +	ret = rte_cfgfile_num_sections(cfgfile, NULL, 0);
> +	TEST_ASSERT(ret == 1, "Unexpected number of sections: %d", ret);
> +
> +	ret = rte_cfgfile_has_section(cfgfile, "GLOBAL");
> +	TEST_ASSERT(ret, "global section missing");
> +
> +	ret = rte_cfgfile_section_num_entries(cfgfile, "GLOBAL");
> +	TEST_ASSERT(ret == 1, "GLOBAL unexpected number of entries: %d",
> ret);
> +
> +	value = rte_cfgfile_get_entry(cfgfile, "GLOBAL", "key");
> +	TEST_ASSERT(strcmp("value", value) == 0,
> +		    "key unexpected value: %s", value);
> +
> +	ret = rte_cfgfile_close(cfgfile);
> +	TEST_ASSERT_SUCCESS(ret, "Failed to close cfgfile");
> +
> +	return 0;
> +}
> +

Can we please add a test to check that error is triggered for keys defined outside any section when the CFG_FLAG_GLOBAL_SECTION is NOT provided to rte_cfgfile_load()? Thank you!

> +static int
>  test_cfgfile_empty_file(void)
>  {
>  	struct rte_cfgfile *cfgfile;
> @@ -198,6 +228,9 @@ test_cfgfile(void)
>  	if (test_cfgfile_missing_section())
>  		return -1;
> 
> +	if (test_cfgfile_global_properties())
> +		return -1;
> +
>  	if (test_cfgfile_empty_file())
>  		return -1;
> 
> --
> 2.12.1

Re: [PATCH v3 2/6] cfgfile: add support for global properties section

[Legacy, Allain]

> -----Original Message-----
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> Sent: Wednesday, March 29, 2017 5:33 AM

<...>
> 
> Can we please add a test to check that error is triggered for keys defined
> outside any section when the CFG_FLAG_GLOBAL_SECTION is NOT provided
> to rte_cfgfile_load()? Thank you!

Already included.   See test_cfgfile_missing_section().