From: Bruce Richardson <bruce.richardson@intel.com>
To: Jacek Piasecki <jacekx.piasecki@intel.com>
Cc: dev@dpdk.org, deepak.k.jain@intel.com, michalx.k.jastrzebski@intel.com
Subject: Re: [dpdk-dev] [PATCH 1/3] cfgfile: add new API functions
Date: Wed, 31 May 2017 15:20:00 +0100 [thread overview]
Message-ID: <20170531141959.GA37544@bricha3-MOBL3.ger.corp.intel.com> (raw)
In-Reply-To: <1496133037-3014-2-git-send-email-jacekx.piasecki@intel.com>
On Tue, May 30, 2017 at 10:30:35AM +0200, Jacek Piasecki wrote:
> Extend existing cfgfile library with providing new API functions:
> rte_cfgfile_create() - create new cfgfile object
> rte_cfgfile_add_section() - add new section to existing cfgfile object
> rte_cfgfile_add_entry() - add new entry to existing cfgfile object
> in specified section
> This modification allows to create a cfgfile on runtime and
> opens up the possibility to have applications dynamically build up
> a proper DPDK configuration, rather than having to have a pre-existing one.
>
> Signed-off-by: Jacek Piasecki <jacekx.piasecki@intel.com>
Hi,
There seems to be some additional work in this patch apart from what is
described above, which may go into a separate patchset to improve
readability. For example, the dependency on librte_eal (apart from one
header) is removed, so those changes could go in a separate patch.
Also, you might consider moving the rework of the load API to a separate
patch, i.e. have one patch add the new APIs, and then a second patch
rework existing load code to be simpler using those APIs.
Other comments inline below.
/Bruce
> ---
> lib/librte_cfgfile/Makefile | 1 +
> lib/librte_cfgfile/rte_cfgfile.c | 293 ++++++++++++++++------------
> lib/librte_cfgfile/rte_cfgfile.h | 50 +++++
> lib/librte_cfgfile/rte_cfgfile_version.map | 9 +
> 4 files changed, 227 insertions(+), 126 deletions(-)
>
<snip>
>
> +struct rte_cfgfile *
> +rte_cfgfile_create(int flags)
> +{
> + int allocated_sections = CFG_ALLOC_SECTION_BATCH;
> + struct rte_cfgfile *cfg = NULL;
> +
> + cfg = malloc(sizeof(*cfg) + sizeof(cfg->sections[0]) *
> + allocated_sections);
> + if (cfg == NULL) {
> + printf("Error - no memory to create cfgfile\n");
> + return NULL;
> + }
> +
> + memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
> +
> + cfg->flags = flags;
> + cfg->num_sections = 0;
> +
> + if (flags & CFG_FLAG_GLOBAL_SECTION)
> + rte_cfgfile_add_section(&cfg, "GLOBAL");
> + return cfg;
> +}
> +
> +int
> +rte_cfgfile_add_section(struct rte_cfgfile **cfgfile, const char *sectionname)
> +{
> + struct rte_cfgfile *cfg = *cfgfile;
> + int curr_section = cfg->num_sections - 1;
Consider removing this variable, I don't know if it's neccessary. It
certainly shouldn't be assigned here using "- 1" and then incremented
unconditionally later on. Throughout the function cfg->num_sections can
be used instead.
> + int allocated_sections = 0;
> +
> + /* check if given section already exist */
> + if (rte_cfgfile_has_section(cfg, sectionname) != 0) {
> + printf("Given section '%s' already exist\n", sectionname);
> + return 0;
I think this should return an error code, -EEXIST.
However, I believe we have support for multiple sections with the same
name - see rte_cfgfile_section_entries_by_entries - so this check will
break that support.
> + }
> + /* calculate allocated sections from number of sections */
> + if ((cfg->num_sections) != 0)
> + allocated_sections = (cfg->num_sections/
> + CFG_ALLOC_SECTION_BATCH + 1) * CFG_ALLOC_SECTION_BATCH;
> +
Rather than compute this value each time, just add a field to the
structure indicating how many are allocated. It should be less error
prone.
> + curr_section++;
> + /* resize overall struct if we don't have room for more sections */
> + if (curr_section == allocated_sections) {
> + allocated_sections += CFG_ALLOC_SECTION_BATCH;
> + struct rte_cfgfile *n_cfg = realloc(cfg,
> + sizeof(*cfg) + sizeof(cfg->sections[0])
> + * allocated_sections);
> + if (n_cfg == NULL)
> + return -ENOMEM;
> + *cfgfile = n_cfg;
> + cfg = *cfgfile;
We should change the definition of rte_config structure to have a
pointer to the sections, rather than having them tacked on at the end.
This would mean that you could just realloc the sections themselves, not
the whole structure. It therefore means that the function can take a
regular struct ptr param, rather than a ptr to struct ptr.
> + }
> + /* allocate space for new section */
> + cfg->sections[curr_section] = malloc(
> + sizeof(*cfg->sections[0]) +
> + sizeof(cfg->sections[0]->entries[0]) *
> + CFG_ALLOC_ENTRY_BATCH);
> +
> + if (cfg->sections[curr_section] == NULL)
> + return -ENOMEM;
> +
> + snprintf(cfg->sections[curr_section]->name,
> + sizeof(cfg->sections[0]->name), "%s", sectionname);
> +
> + cfg->sections[curr_section]->num_entries = 0;
> + cfg->num_sections = curr_section + 1;
Don't need to use curr_section here, just do cfg->num_sections++;
cfg->num_sections++;
> + return 0;
> +}
> +
> +static int
> +_get_section_index(struct rte_cfgfile *cfgfile, const char *sectionname) {
> + int i;
> +
> + for (i = 0; i < cfgfile->num_sections; i++) {
> + if (strncmp(cfgfile->sections[i]->name, sectionname,
> + sizeof(cfgfile->sections[0]->name)) == 0)
> + return i;
> + }
> + return -1;
> +}
This shouldn't be necessary, as we can use existing _get_section()
function instead. [Or else make one use the other]
> +
> +static signed int
> +_add_entry(struct rte_cfgfile *cfgfile, const signed int curr_section,
> + const char *entryname, const char *entryvalue)
> +{
> + int allocated_entries = 0;
> + int curr_entry = cfgfile->sections[curr_section]->num_entries - 1;
> +
> + /* calculate allocated entries from number of entries */
> + if ((curr_entry + 1) != 0)
> + allocated_entries = ((curr_entry + 1)/
> + CFG_ALLOC_ENTRY_BATCH + 1) * CFG_ALLOC_ENTRY_BATCH;
> +
As with the add_section, we don't need a curr_entry variable, and also
add a new struct member to track the number of allocated elements.
> + curr_entry++;
> + struct rte_cfgfile_section *sect = cfgfile->sections[curr_section];
> +
> + sect->entries[curr_entry] = malloc(sizeof(*sect->entries[0]));
> +
> + if (curr_entry == allocated_entries) {
> + allocated_entries += CFG_ALLOC_ENTRY_BATCH;
> + struct rte_cfgfile_section *n_sect = realloc(
> + sect, sizeof(*sect) +
> + sizeof(sect->entries[0]) *
> + allocated_entries);
> + if (n_sect == NULL)
> + return -ENOMEM;
> + sect = cfgfile->sections[curr_section] = n_sect;
> + }
> +
> + if (sect->entries[curr_entry] == NULL) {
> + cfgfile->num_sections = curr_section + 1;
> + if (curr_section >= 0) {
> + cfgfile->sections[curr_section]->num_entries =
> + curr_entry + 1;
> + return -ENOMEM;
> + }
> + }
> +
Do we need to check for the existence of an entry with that name here?
> + struct rte_cfgfile_entry *entry = sect->entries[curr_entry];
> +
> + snprintf(entry->name, sizeof(entry->name), "%s", entryname);
> + snprintf(entry->value, sizeof(entry->value), "%s", entryvalue);
> + _strip(entry->name, strnlen(entry->name, sizeof(entry->name)));
> + _strip(entry->value, strnlen(entry->value, sizeof(entry->value)));
> +
> + cfgfile->sections[curr_section]->num_entries = curr_entry + 1;
> +
> + return 0;
> +};
> +
> +int rte_cfgfile_add_entry(struct rte_cfgfile *cfgfile, const char *sectionname,
> + const char *entryname, const char *entryvalue)
> +{
> + int curr_section;
> +
> + /* check if given entry in specified section already exist */
> + if (rte_cfgfile_has_entry(cfgfile, sectionname, entryname) != 0)
> + return 0;
> +
> + /* search for section index by sectionname */
> + curr_section = _get_section_index(cfgfile, sectionname);
> +
> + if (curr_section < 0)
> + return -EINVAL;
> +
> + return _add_entry(cfgfile, curr_section, entryname, entryvalue);
rename function to "add_section_entry", perhaps. If you use
_get_section() function instead of the _get_section_index() one, you can
potentially pass in the section by pointer and not pass in the cfgfile
pointer at all.
> +}
>
<snip>
next prev parent reply other threads:[~2017-05-31 14:20 UTC|newest]
Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-30 8:30 [dpdk-dev] [PATCH 0/3] Add support for using a config file for DPDK Jacek Piasecki
2017-05-30 8:30 ` [dpdk-dev] [PATCH 1/3] cfgfile: add new API functions Jacek Piasecki
2017-05-31 14:20 ` Bruce Richardson [this message]
2017-05-31 14:22 ` Bruce Richardson
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 0/7] Add support for using a config file for DPDK Jacek Piasecki
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 1/7] cfgfile: remove EAL dependency Jacek Piasecki
2017-06-26 13:12 ` Dumitrescu, Cristian
2017-06-27 10:26 ` [dpdk-dev] [PATCH v3 0/4] Rework cfgfile API to enable apps config file support Jacek Piasecki
2017-06-27 10:26 ` [dpdk-dev] [PATCH v3 1/4] cfgfile: remove EAL dependency Jacek Piasecki
2017-06-30 9:44 ` Bruce Richardson
2017-06-30 11:16 ` Bruce Richardson
2017-06-27 10:26 ` [dpdk-dev] [PATCH v3 2/4] cfgfile: add new functions to API Jacek Piasecki
2017-06-30 9:55 ` Bruce Richardson
2017-06-30 10:28 ` Bruce Richardson
2017-06-27 10:26 ` [dpdk-dev] [PATCH v3 3/4] cfgfile: rework of load function Jacek Piasecki
2017-06-30 11:18 ` Bruce Richardson
2017-06-27 10:26 ` [dpdk-dev] [PATCH v3 4/4] test/cfgfile: add new unit test Jacek Piasecki
2017-06-30 11:20 ` [dpdk-dev] [PATCH v3 0/4] Rework cfgfile API to enable apps config file support Bruce Richardson
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 2/7] cfgfile: add new functions to API Jacek Piasecki
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 3/7] cfgfile: rework of load function Jacek Piasecki
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 4/7] test/cfgfile: add new unit test Jacek Piasecki
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 5/7] eal: add functions parsing EAL arguments Jacek Piasecki
2017-06-27 10:52 ` [dpdk-dev] [PATCH v3 0/3] EAL change for using a config file for DPDK Jacek Piasecki
2017-06-27 10:52 ` [dpdk-dev] [PATCH v3 1/3] eal: add functions parsing EAL arguments Jacek Piasecki
2017-06-30 16:04 ` Bruce Richardson
2017-07-10 12:44 ` [dpdk-dev] [PATCH v4 0/5] Rework cfgfile API to enable apps config file support Jacek Piasecki
2017-07-10 12:44 ` [dpdk-dev] [PATCH v4 1/5] cfgfile: remove EAL dependency Jacek Piasecki
2017-08-30 17:58 ` Bruce Richardson
2017-07-10 12:44 ` [dpdk-dev] [PATCH v4 2/5] cfgfile: change existing API functions Jacek Piasecki
2017-08-30 20:07 ` Bruce Richardson
2017-07-10 12:44 ` [dpdk-dev] [PATCH v4 3/5] cfgfile: add new functions to API Jacek Piasecki
2017-08-30 20:18 ` Bruce Richardson
2017-07-10 12:44 ` [dpdk-dev] [PATCH v4 4/5] cfgfile: rework of load function Jacek Piasecki
2017-08-30 20:24 ` Bruce Richardson
2017-07-10 12:44 ` [dpdk-dev] [PATCH v4 5/5] test/cfgfile: add new unit test Jacek Piasecki
2017-08-30 20:25 ` Bruce Richardson
2017-09-04 9:21 ` Bruce Richardson
2017-09-04 9:30 ` Bruce Richardson
2017-09-15 13:56 ` Thomas Monjalon
2017-09-18 13:49 ` Jastrzebski, MichalX K
2017-07-10 15:13 ` [dpdk-dev] [PATCH v4 0/5] Rework cfgfile API to enable apps config file support Thomas Monjalon
2017-07-20 21:48 ` Thomas Monjalon
2017-07-10 12:51 ` [dpdk-dev] [PATCH v4 0/3] EAL change for using a config file for DPDK Kuba Kozak
2017-07-10 12:51 ` [dpdk-dev] [PATCH v4 1/3] eal: add functions parsing EAL arguments Kuba Kozak
2017-07-13 9:26 ` [dpdk-dev] [PATCH v5 0/3] EAL change for using a config file for DPDK Kuba Kozak
2017-07-13 10:07 ` Kuba Kozak
2017-07-13 10:07 ` [dpdk-dev] [PATCH v5 1/3] eal: add functions parsing EAL arguments Kuba Kozak
2017-07-13 10:07 ` [dpdk-dev] [PATCH v5 2/3] app/testpmd: add parse options from cfg file Kuba Kozak
2017-07-13 10:07 ` [dpdk-dev] [PATCH v5 3/3] app/testpmd: add parse options from JSON " Kuba Kozak
2019-01-23 19:31 ` [dpdk-dev] [PATCH v5 0/3] EAL change for using a config file for DPDK Ferruh Yigit
2019-01-23 20:26 ` Thomas Monjalon
2019-01-24 13:54 ` Ferruh Yigit
2019-01-24 14:32 ` Thomas Monjalon
2019-01-24 14:46 ` Ferruh Yigit
2019-01-24 16:06 ` Thomas Monjalon
2019-01-24 16:18 ` Ferruh Yigit
2019-01-24 17:45 ` Thomas Monjalon
2019-01-28 14:43 ` Ferruh Yigit
2017-07-10 12:51 ` [dpdk-dev] [PATCH v4 2/3] app/testpmd: add parse options from cfg file Kuba Kozak
2017-07-10 12:51 ` [dpdk-dev] [PATCH v4 3/3] app/testpmd: add parse options from JSON " Kuba Kozak
2017-06-27 10:52 ` [dpdk-dev] [PATCH v3 2/3] app/testpmd: changed example to parse options from " Jacek Piasecki
2017-06-27 10:52 ` [dpdk-dev] [PATCH v3 3/3] app/testpmd: add parse arguments from JSON config file Jacek Piasecki
2017-07-05 0:00 ` [dpdk-dev] [PATCH v3 0/3] EAL change for using a config file for DPDK Thomas Monjalon
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 6/7] app/testpmd: changed example to parse options from cfg file Jacek Piasecki
2017-06-26 10:59 ` [dpdk-dev] [PATCH v2 7/7] app/testpmd: add parse arguments from JSON config file Jacek Piasecki
2017-05-30 8:30 ` [dpdk-dev] [PATCH 2/3] eal: add functions parsing EAL arguments Jacek Piasecki
2017-05-31 15:46 ` Bruce Richardson
2017-05-30 8:30 ` [dpdk-dev] [PATCH 3/3] app/testpmd: changed example to parse options from cfg file Jacek Piasecki
2017-06-20 2:13 ` Wu, Jingjing
2017-06-20 10:10 ` Kozak, KubaX
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=20170531141959.GA37544@bricha3-MOBL3.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=deepak.k.jain@intel.com \
--cc=dev@dpdk.org \
--cc=jacekx.piasecki@intel.com \
--cc=michalx.k.jastrzebski@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).