From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: "Legacy, Allain (Wind River)" <allain.legacy@windriver.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "yuanhan.liu@linux.intel.com" <yuanhan.liu@linux.intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 3/6] cfgfile: configurable comment character
Date: Mon, 27 Mar 2017 11:19:27 +0000 [thread overview]
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891265277CEEF@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <1489065060-98370-4-git-send-email-allain.legacy@windriver.com>
> -----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(¶ms);
> + params.flags |= 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 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(¶ms);
> +
> + /* override comment character */
> + params.comment_character = '#';
> +
> + cfgfile = rte_cfgfile_load_with_params(CFG_FILES_ETC
> "/sample2.ini",
> + ¶ms);
> + 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
next prev parent reply other threads:[~2017-03-27 11:19 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-02 19:29 [dpdk-dev] [PATCH 0/5] librte_cfgfile enhancement Allain Legacy
2017-03-02 19:29 ` [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character Allain Legacy
2017-03-02 21:10 ` Bruce Richardson
2017-03-02 21:22 ` Legacy, Allain
2017-03-03 0:53 ` Yuanhan Liu
2017-03-03 11:17 ` Dumitrescu, Cristian
2017-03-03 11:31 ` Legacy, Allain
2017-03-03 12:07 ` Dumitrescu, Cristian
2017-03-03 12:14 ` Legacy, Allain
2017-03-03 12:17 ` Dumitrescu, Cristian
2017-03-03 12:18 ` Legacy, Allain
2017-03-03 12:52 ` Dumitrescu, Cristian
2017-03-03 12:10 ` Bruce Richardson
2017-03-03 12:17 ` Legacy, Allain
2017-03-03 13:10 ` Bruce Richardson
2017-03-02 19:29 ` [dpdk-dev] [PATCH 2/5] cfgfile: cfg object not initialized after allocation Allain Legacy
2017-03-02 19:29 ` [dpdk-dev] [PATCH 3/5] cfgfile: add support for unamed global section Allain Legacy
2017-03-03 10:53 ` Dumitrescu, Cristian
2017-03-03 11:03 ` Legacy, Allain
2017-03-03 11:06 ` Dumitrescu, Cristian
2017-03-03 11:15 ` Legacy, Allain
2017-03-03 11:18 ` Dumitrescu, Cristian
2017-03-02 19:29 ` [dpdk-dev] [PATCH 4/5] cfgfile: use strnlen to constrain memchr search Allain Legacy
2017-03-02 19:29 ` [dpdk-dev] [PATCH 5/5] cfgfile: increase local buffer size for max name and value Allain Legacy
2017-03-09 13:46 ` Wiles, Keith
2017-03-09 15:16 ` Legacy, Allain
2017-03-09 15:23 ` Wiles, Keith
2017-03-09 13:10 ` [dpdk-dev] [PATCH v2 0/6] librte_cfgfile enhancements Allain Legacy
2017-03-09 13:10 ` [dpdk-dev] [PATCH v2 1/6] test: basic unit tests for cfgfile Allain Legacy
2017-03-09 13:10 ` [dpdk-dev] [PATCH v2 2/6] cfgfile: add support for unamed global section Allain Legacy
2017-03-09 13:10 ` [dpdk-dev] [PATCH v2 3/6] cfgfile: configurable comment character Allain Legacy
2017-03-27 11:19 ` Dumitrescu, Cristian [this message]
2017-03-09 13:10 ` [dpdk-dev] [PATCH v2 4/6] cfgfile: use strnlen to constrain memchr search Allain Legacy
2017-03-09 13:10 ` [dpdk-dev] [PATCH v2 5/6] cfgfile: increase local buffer size for max name and value Allain Legacy
2017-03-09 13:11 ` [dpdk-dev] [PATCH v2 6/6] cfgfile: add support for empty value string Allain Legacy
2017-03-27 10:54 ` Dumitrescu, Cristian
2017-03-27 11:12 ` Legacy, Allain
2017-03-27 11:24 ` Dumitrescu, Cristian
2017-03-27 11:15 ` Legacy, Allain
2017-03-28 8:29 ` [dpdk-dev] [PATCH v2 0/6] librte_cfgfile enhancements Thomas Monjalon
2017-03-28 9:18 ` Bruce Richardson
2017-03-28 9:22 ` Bruce Richardson
2017-03-28 9:41 ` Thomas Monjalon
2017-03-28 9:58 ` Dumitrescu, Cristian
2017-03-28 10:12 ` Thomas Monjalon
2017-03-28 10:20 ` Dumitrescu, Cristian
2017-03-28 15:24 ` Bruce Richardson
2017-03-28 15:41 ` Thomas Monjalon
2017-03-28 15:42 ` Bruce Richardson
2017-03-28 16:44 ` [dpdk-dev] [PATCH v3 " Allain Legacy
2017-03-28 16:44 ` [dpdk-dev] [PATCH v3 1/6] test: basic unit tests for cfgfile Allain Legacy
2017-03-28 16:44 ` [dpdk-dev] [PATCH v3 3/6] cfgfile: add support for configurable comment character Allain Legacy
2017-03-28 16:44 ` [dpdk-dev] [PATCH v3 4/6] cfgfile: use strnlen to constrain memchr search Allain Legacy
2017-03-28 16:44 ` [dpdk-dev] [PATCH v3 6/6] cfgfile: add support for empty value string Allain Legacy
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 0/6] librte_cfgfile enhancements Allain Legacy
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 1/6] test: basic unit tests for cfgfile Allain Legacy
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 2/6] cfgfile: add support for global properties section Allain Legacy
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 3/6] cfgfile: add support for configurable comment character Allain Legacy
2017-03-31 10:08 ` Thomas Monjalon
2017-03-31 11:08 ` Legacy, Allain
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 4/6] cfgfile: use strnlen to constrain memchr search Allain Legacy
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 5/6] cfgfile: increase local buffer size for max name and value Allain Legacy
2017-03-30 18:54 ` [dpdk-dev] [PATCH v4 6/6] cfgfile: add support for empty value string Allain Legacy
2017-03-31 8:57 ` [dpdk-dev] [PATCH v4 0/6] librte_cfgfile enhancements Dumitrescu, Cristian
2017-03-31 13:51 ` [dpdk-dev] [PATCH v5 " Allain Legacy
2017-03-31 13:51 ` [dpdk-dev] [PATCH v5 1/6] test: basic unit tests for cfgfile Allain Legacy
2017-03-31 13:51 ` [dpdk-dev] [PATCH v5 2/6] cfgfile: add support for global properties section Allain Legacy
2017-03-31 13:52 ` [dpdk-dev] [PATCH v5 3/6] cfgfile: add support for configurable comment character Allain Legacy
2017-03-31 13:52 ` [dpdk-dev] [PATCH v5 4/6] cfgfile: use strnlen to constrain memchr search Allain Legacy
2017-03-31 13:52 ` [dpdk-dev] [PATCH v5 5/6] cfgfile: increase local buffer size for max name and value Allain Legacy
2017-03-31 13:52 ` [dpdk-dev] [PATCH v5 6/6] cfgfile: add support for empty value string Allain Legacy
2017-04-04 14:23 ` [dpdk-dev] [PATCH v5 0/6] librte_cfgfile enhancements Thomas Monjalon
[not found] ` <20170329004737.44249-1-allain.legacy@windriver.com>
2017-03-29 0:47 ` [dpdk-dev] [PATCH v3 1/6] test: basic unit tests for cfgfile Allain Legacy
2017-03-29 0:47 ` [dpdk-dev] [PATCH v3 3/6] cfgfile: add support for configurable comment character Allain Legacy
2017-03-29 9:22 ` Dumitrescu, Cristian
2017-03-29 11:31 ` Legacy, Allain
2017-03-29 0:47 ` [dpdk-dev] [PATCH v3 5/6] cfgfile: increase local buffer size for max name and value Allain Legacy
[not found] ` <20170329004737.44249-7-allain.legacy@windriver.com>
2017-03-29 9:31 ` [dpdk-dev] [PATCH v3 6/6] cfgfile: add support for empty value string Dumitrescu, Cristian
2017-03-29 11:33 ` Legacy, Allain
[not found] ` <20170329004737.44249-3-allain.legacy@windriver.com>
2017-03-29 9:33 ` [dpdk-dev] [PATCH v3 2/6] cfgfile: add support for global properties section Dumitrescu, Cristian
2017-03-29 11:35 ` Legacy, Allain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3EB4FA525960D640B5BDFFD6A3D891265277CEEF@IRSMSX108.ger.corp.intel.com \
--to=cristian.dumitrescu@intel.com \
--cc=allain.legacy@windriver.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=yuanhan.liu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).