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

[Bruce Richardson]

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