[dpdk-dev,v2,6/6] cfgfile: add support for empty value string
Checks
Commit Message
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(-)
Comments
> -----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?
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?
> -----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
> -----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?
>
@@ -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,
@@ -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;
@@ -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
@@ -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