From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dumitrescu, Cristian" Subject: Re: [PATCH v2 3/6] cfgfile: configurable comment character Date: Mon, 27 Mar 2017 11:19:27 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891265277CEEF@IRSMSX108.ger.corp.intel.com> References: <1488482971-170522-1-git-send-email-allain.legacy@windriver.com> <1489065060-98370-1-git-send-email-allain.legacy@windriver.com> <1489065060-98370-4-git-send-email-allain.legacy@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "yuanhan.liu@linux.intel.com" , "dev@dpdk.org" To: "Legacy, Allain (Wind River)" , "Richardson, Bruce" Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id E82AEFA78 for ; Mon, 27 Mar 2017 13:19:33 +0200 (CEST) In-Reply-To: <1489065060-98370-4-git-send-email-allain.legacy@windriver.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Allain Legacy [mailto:allain.legacy@windriver.com] > Sent: Thursday, March 9, 2017 1:11 PM > To: Richardson, Bruce ; Dumitrescu, Cristian > > Cc: yuanhan.liu@linux.intel.com; dev@dpdk.org > Subject: [PATCH v2 3/6] cfgfile: configurable comment character >=20 > 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. >=20 > 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. >=20 > 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 allo= w > for additional arguments to be introduced in the future. >=20 > Signed-off-by: Allain Legacy > --- > 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 >=20 > diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cf= gfile.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; > } >=20 > +void > +rte_cfgfile_init_parameters(struct rte_cfgfile_parameters *params) > +{ > + memset(params, 0, sizeof(*params)); > + params->comment_character =3D > 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 variab= le 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(¶ms); > + params.flags |=3D flags; > + > + return rte_cfgfile_load_with_params(filename, ¶ms); > +} > + > +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 structu= re having just single field for now. > +{ > int allocated_sections =3D CFG_ALLOC_SECTION_BATCH; > int allocated_entries =3D 0; > int curr_section =3D -1; > @@ -107,7 +127,7 @@ struct rte_cfgfile * >=20 > memset(cfg->sections, 0, sizeof(cfg->sections[0]) * > allocated_sections); >=20 > - if (flags & CFG_FLAG_GLOBAL_SECTION) { > + if (params->flags & CFG_FLAG_GLOBAL_SECTION) { > curr_section =3D 0; > allocated_entries =3D CFG_ALLOC_ENTRY_BATCH; > cfg->sections[curr_section] =3D malloc( > @@ -132,7 +152,7 @@ struct rte_cfgfile * > "Check if line too long\n", lineno); > goto error1; > } > - pos =3D memchr(buffer, ';', sizeof(buffer)); > + pos =3D memchr(buffer, params->comment_character, > sizeof(buffer)); > if (pos !=3D NULL) { > *pos =3D '\0'; > len =3D pos - buffer; > @@ -242,7 +262,7 @@ struct rte_cfgfile * > } > } Per previous feedback, please check the value of the comment_character agai= nst invalid options. I suggest creating a new static function in the file for validating the par= ameter 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/20= 17-March/059159.html): I does not make sense to allow letters, numbers, formatting characters (tab= s, 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 al= low them. The API should not allow options that do not make sense. > fclose(f); > - cfg->flags =3D flags; > + cfg->flags =3D params->flags; > cfg->num_sections =3D curr_section + 1; > /* curr_section will still be -1 if we have an empty file */ > if (curr_section >=3D 0) > diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cf= gfile.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 */ > }; >=20 > +/** 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 t= he 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 d= efined > @@ -74,6 +80,17 @@ struct rte_cfgfile_entry { > #define CFG_FLAG_GLOBAL_SECTION (1 << 0) > /**@} */ >=20 > +/** 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); >=20 > /** > + * 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 @@ > } >=20 > static int > +test_cfgfile_sample2(void) > +{ > + struct rte_cfgfile_parameters params; > + struct rte_cfgfile *cfgfile; > + int ret; > + > + /* setup default */ > + rte_cfgfile_init_parameters(¶ms); > + > + /* override comment character */ > + params.comment_character =3D '#'; > + > + cfgfile =3D rte_cfgfile_load_with_params(CFG_FILES_ETC > "/sample2.ini", > + ¶ms); > + TEST_ASSERT_NOT_NULL(cfgfile, "Failed to parse sample2.ini"); > + > + ret =3D _test_cfgfile_sample(cfgfile); > + TEST_ASSERT_SUCCESS(ret, "Failed to validate sample file: %d", ret); > + > + ret =3D 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; >=20 > + if (test_cfgfile_sample2()) > + return -1; > + > if (test_cfgfile_invalid_section_header()) > return -1; >=20 > 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=3Dvalue1 > + > +[section2] > +# this is section 2 > +#key1=3Dvalue1 > +key2=3Dvalue2 > +key3=3Dvalue3 # this is key3 > +ignore-missing-separator > -- > 1.8.3.1 We do not want to allow in DPDK config files that use comment characters th= at 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