From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490613574; x=1522149574; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=0L3iNswmRhgbiF9QuI3NuKJFsKN0A+pfPrLvXHXvE5A=; b=hPTOIRFg/8cxXItZ/teVmB8nUtjohe2cI20ih4Rx6wC7oeMC8PAZjYf+ /CUeOhGub0CehO/SX/L9c4d+BPQgKw==; Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Mar 2017 04:19:32 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,231,1486454400"; d="scan'208";a="240692678" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga004.fm.intel.com with ESMTP; 27 Mar 2017 04:19:28 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.239]) by IRSMSX107.ger.corp.intel.com ([169.254.10.107]) with mapi id 14.03.0319.002; Mon, 27 Mar 2017 12:19:28 +0100 From: "Dumitrescu, Cristian" To: "Legacy, Allain (Wind River)" , "Richardson, Bruce" CC: "yuanhan.liu@linux.intel.com" , "dev@dpdk.org" Thread-Topic: [PATCH v2 3/6] cfgfile: configurable comment character Thread-Index: AQHSmNazKCXYzrEFfU25CB/yJzDuLqGon24g 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> In-Reply-To: <1489065060-98370-4-git-send-email-allain.legacy@windriver.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjMyOGZjMjctYTRmNS00ODllLWIyN2UtMDU1ODNkYzkwOWYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Imh3VTBGaElHa3Q3RVJLVE9cLzBmTGNodStpb0Z6SGd1ZWNRWFFzR1M0dys4PSJ9 x-ctpclassification: CTP_IC x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 3/6] cfgfile: configurable comment character X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Mar 2017 11:19:34 -0000 > -----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