[dpdk-dev,1/3] cfgfile: add new API functions

  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>
---
 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(-)
  

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>
  

On Wed, May 31, 2017 at 03:20:00PM +0100, Bruce Richardson wrote:
> 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.

s/a separate patchset/separate patches/

> 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.
>
  

This patchset introduce a mechanism for running dpdk application with parameters
provided by configuration file. New API for cfgfile library allows to create
a cfgfile at runtime, add new section and add entry in a section - opens up
the possibility to have applications dynamically build up a proper
DPDK configuration, rather than having to have a pre-existing one.

A new API for EAL takes a config file data type - either loaded from file,
or built up programmatically in the application - and extracts DPDK parameters
from it to be used when eal init is called. This allows apps to have
an alternative method to configure EAL, other than via command-line parameters.

Testpmd application is used to the demonstrate the new eal API.
If a --cfgfile-path <path> option is passed into command line non EAL section,
then the file is loaded and used by app. If a file called config.ini
is present in current working directory, and no --cfgfile-path option is
passed in, config.ini file will be loaded and used by app.

Last patch demonstrates the usage of JSON file format insted of config.ini
JSON file can be called the same way as above, thru --cfgfile-path <path>

---
v2:
  lib eal:
  	Rework of rte_eal_configure(struct rte_cfgfile *cfg, char *prgname).
	Now this function load data from cfg structure and did initial
	initialization of EAL arguments. Vdev argument are stored in different
	subsections eg. DPDK.vdev0, DPDK.vdev1 etc. After execution of this
	function it is necessary to call rte_eal_init to complete EAL
	initialization. There is no more merging arguments from different
	sources (cfg file and command line).
  	Added non_eal_configure to testpmd application.
	Function maintain the same functionality as rte_eal_configure but
	for non-eal arguments. 
  	Added config JSON feature to testpmd last patch from patchset contain
	example showing use of .json configuration files.

  lib cfgfile:
  	Rework of add_section(), add_entry() new implementation
  	New members allocated_entries/sections, free_entries/sections
	in rte_cfgfile structure, change in array of pointers
	**sections, **entries instead of *sections[], *entries[]
  	Add  set_entry() to update/overwrite already existing entry in cfgfile
	struct
  	Add save() function to save on disc cfgfile structure in INI format
  	Rework of existing load() function  simplifying the code
  	Add unit test realloc_sections() in TEST app for testing realloc/malloc
	of new API functions, add test for save() function

Jacek Piasecki (3):
  cfgfile: remove EAL dependency
  cfgfile: add new functions to API
  test/cfgfile: add new unit test

Kuba Kozak (4):
  cfgfile: rework of load function
  eal: add functions parsing EAL arguments
  app/testpmd: changed example to parse options from cfg file
  app/testpmd: add parse arguments from JSON config file

 app/test-pmd/Makefile                            |    6 +
 app/test-pmd/config.ini                          |   24 +
 app/test-pmd/config.json                         |   33 +
 app/test-pmd/parameters.c                        | 1193 +++++++++++++---------
 app/test-pmd/testpmd.c                           |  159 ++-
 app/test-pmd/testpmd.h                           |    3 +-
 config/common_base                               |    6 +
 lib/Makefile                                     |    6 +-
 lib/librte_cfgfile/Makefile                      |    1 +
 lib/librte_cfgfile/rte_cfgfile.c                 |  397 ++++---
 lib/librte_cfgfile/rte_cfgfile.h                 |   76 ++
 lib/librte_cfgfile/rte_cfgfile_version.map       |   11 +
 lib/librte_eal/bsdapp/eal/Makefile               |    4 +
 lib/librte_eal/bsdapp/eal/eal.c                  |  249 ++++-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map    |    4 +
 lib/librte_eal/common/eal_common_cpuflags.c      |   14 +-
 lib/librte_eal/common/eal_common_lcore.c         |   11 +-
 lib/librte_eal/common/eal_common_options.c       |    5 +
 lib/librte_eal/common/include/rte_eal.h          |   21 +
 lib/librte_eal/linuxapp/eal/Makefile             |    3 +
 lib/librte_eal/linuxapp/eal/eal.c                |  353 +++++--
 lib/librte_eal/linuxapp/eal/rte_eal_version.map  |    4 +
 mk/rte.app.mk                                    |    2 +-
 test/test/test_cfgfile.c                         |   40 +
 test/test/test_cfgfiles/etc/realloc_sections.ini |  128 +++
 25 files changed, 1983 insertions(+), 770 deletions(-)
 create mode 100644 app/test-pmd/config.ini
 create mode 100644 app/test-pmd/config.json
 create mode 100644 test/test/test_cfgfiles/etc/realloc_sections.ini