[dpdk-dev,v3,2/4] cfgfile: add new functions to API

  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
rte_cfgfile_set_entry() - update existing entry in cfgfile object
rte_cfgfile_save() - save existing cfgfile object to INI file

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/rte_cfgfile.c           | 249 +++++++++++++++++++++++++++--
 lib/librte_cfgfile/rte_cfgfile.h           |  76 +++++++++
 lib/librte_cfgfile/rte_cfgfile_version.map |  11 ++
 3 files changed, 323 insertions(+), 13 deletions(-)
  

On Tue, Jun 27, 2017 at 12:26:48PM +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
> rte_cfgfile_set_entry() - update existing entry in cfgfile object
> rte_cfgfile_save() - save existing cfgfile object to INI file
> 
> 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/rte_cfgfile.c           | 249 +++++++++++++++++++++++++++--
>  lib/librte_cfgfile/rte_cfgfile.h           |  76 +++++++++
>  lib/librte_cfgfile/rte_cfgfile_version.map |  11 ++
>  3 files changed, 323 insertions(+), 13 deletions(-)
> 
<snip>
> +/**
> + * Add entry to specified section in cfgfile instance.
> + *
> + * @param cfg
> + *   Pointer to the cfgfile structure.
> + * @param sectionname
> + *   Given section name to add an entry.
> + * @param entryname
> + *   Entry name to add.
> + * @entryvalue

missing "param" here. Building the docs throws an error on this.

+/home/bruce/dpdk.org/lib/librte_cfgfile/rte_cfgfile.h:161: warning: Found unknown command `\entryvalue'
+/home/bruce/dpdk.org/lib/librte_cfgfile/rte_cfgfile.h:166: warning: The following parameters of rte_cfgfile_add_entry(struct rte_cfgfile *cfg, const char *sectionname, const char *entryname, const char *entryvalue) are not documented:
+  parameter 'entryvalue'

> + *   Entry value to add.
> + * @return
> + *   0 on success, -EEXIST if entry already exist, -EINVAL if bad argument
> + */
> +int rte_cfgfile_add_entry(struct rte_cfgfile *cfg,
> +		const char *sectionname, const char *entryname,
> +		const char *entryvalue);
> +
  

On Tue, Jun 27, 2017 at 12:26:48PM +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
> rte_cfgfile_set_entry() - update existing entry in cfgfile object
> rte_cfgfile_save() - save existing cfgfile object to INI file
> 
> 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>

Some comments inline below on the code.

I also think you might want to split this patch into 2 or 3 to make
review easier. For example:
* Patch to add add_section and add_entry functions
* Patch to add set_entry function
* Patch to add create() and save functions

/Bruce

> ---
>  lib/librte_cfgfile/rte_cfgfile.c           | 249 +++++++++++++++++++++++++++--
>  lib/librte_cfgfile/rte_cfgfile.h           |  76 +++++++++
>  lib/librte_cfgfile/rte_cfgfile_version.map |  11 ++
>  3 files changed, 323 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index c6ae3e3..518b6ab 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -35,6 +35,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <ctype.h>
> +#include <errno.h>
>  #include <rte_common.h>
>  
>  #include "rte_cfgfile.h"
> @@ -42,13 +43,17 @@
>  struct rte_cfgfile_section {
>  	char name[CFG_NAME_LEN];
>  	int num_entries;
> -	struct rte_cfgfile_entry *entries[0];
> +	int free_entries;
> +	int allocated_entries;
> +	struct rte_cfgfile_entry **entries;
>  };

I'm not sure that we need to use an array of pointers here any more. It
might work easier to just store the entries for each section in a single
block, i.e. have the last member just:
	"struct rte_cfgfile_entry *entries;"

Also, allocated_entries value seems unneeded, since it is always equal
to num_entries + free_entries. The place where is seems to be used, when
allocating new entries, free_entries == 0, so allocated_entries ==
num_entries, making allocated entries doubly-unneeded there. Another,
possibly better, alternative might be to remove the "free_entries"
variable. [It's possibly better as you don't need to do an increment and
a decrement on adding an entry, but just do an increment]

>  
>  struct rte_cfgfile {
>  	int flags;
>  	int num_sections;
> -	struct rte_cfgfile_section *sections[0];
> +	int free_sections;
> +	int allocated_sections;
> +	struct rte_cfgfile_section **sections;
>  };

As above: for the entries in a section, we should be ok putting all
sections in a single array, rather than having an array of pointers, and
we don't need both an allocated_sections variable and a free_sections
one.

>  
>  /** when we resize a file structure, how many extra entries
> @@ -104,6 +109,65 @@ _strip(char *str, unsigned len)
>  	return newlen;
>  }
>  
> +static struct rte_cfgfile_section *
> +_get_section(struct rte_cfgfile *cfg, const char *sectionname)
> +{
> +	int i;
> +
> +	for (i = 0; i < cfg->num_sections; i++) {
> +		if (strncmp(cfg->sections[i]->name, sectionname,
> +				sizeof(cfg->sections[0]->name)) == 0)
> +			return cfg->sections[i];
> +	}
> +	return NULL;
> +}
> +
> +static int
> +_add_entry(struct rte_cfgfile_section *section, const char *entryname,
> +		const char *entryvalue)
> +{
> +	int i;
> +
> +	/* resize entry structure if we don't have room for more entries */
> +	if (section->free_entries == 0) {
> +
> +		struct rte_cfgfile_entry **n_entries =
> +				realloc(section->entries,
> +				sizeof(section->entries[0]) *
> +				((section->allocated_entries) +
> +						CFG_ALLOC_ENTRY_BATCH));
> +
> +		if (n_entries == NULL)
> +			return -ENOMEM;
> +
> +		section->entries = n_entries;
> +
> +		for (i = section->allocated_entries;
> +				i < (section->allocated_entries) +
> +						CFG_ALLOC_ENTRY_BATCH; i++) {
> +			section->entries[i] =
> +				malloc(sizeof(struct rte_cfgfile_entry));
> +
> +			if (section->entries[i] == NULL)
> +				return -ENOMEM;
> +		}

Even if you keep the entries array as an array of pointers, you still
don't need to loop here for malloc. Instead just malloc a single block
for CFG_ALLOC_ENTRY_BATCH entries in one go, and set the pointers to
point to values in that block. Too many malloc calls is fragmenting
memory.

> +		section->allocated_entries += CFG_ALLOC_ENTRY_BATCH;
> +		section->free_entries += CFG_ALLOC_ENTRY_BATCH;
> +	}
> +
> +	/* fill up entry fields with key name and value */
> +	struct rte_cfgfile_entry *curr_entry =
> +			section->entries[section->num_entries];
> +
> +	snprintf(curr_entry->name, sizeof(curr_entry->name), "%s", entryname);
> +	snprintf(curr_entry->value, sizeof(curr_entry->value), "%s",
> +								entryvalue);
> +	section->num_entries++;
> +	section->free_entries--;
> +
> +	return 0;
> +}
> +
>  static int
>  rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
>  {
> @@ -332,6 +396,176 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  	return NULL;
>  }
>  
> +struct rte_cfgfile *
> +rte_cfgfile_create(int flags)
> +{
> +	int i, j;
> +	struct rte_cfgfile *cfg = NULL;
> +
> +	cfg = malloc(sizeof(*cfg));
> +	if (cfg == NULL)
> +		return NULL;
> +
> +	memset(cfg, 0, sizeof((*cfg)));
> +
> +	cfg->flags = flags;
> +
> +	/* allocate first batch of sections and entries */
> +	cfg->sections = malloc(sizeof(cfg->sections[0]) *
> +						CFG_ALLOC_SECTION_BATCH);
> +	if (cfg->sections == NULL)
> +		return NULL;
> +
> +	for (i = 0; i < CFG_ALLOC_SECTION_BATCH; i++) {
> +		cfg->sections[i] = malloc(sizeof(struct rte_cfgfile_section));
> +		if (cfg->sections[i] == NULL)
> +			return NULL;
> +
> +		memset(cfg->sections[i], 0,
> +					sizeof(struct rte_cfgfile_section));
> +
> +		cfg->sections[i]->entries =
> +				malloc(sizeof(cfg->sections[i]->entries[0])
> +						* CFG_ALLOC_ENTRY_BATCH);
> +		if (cfg->sections[i]->entries == NULL)
> +			return NULL;
> +
> +		for (j = 0; j < CFG_ALLOC_ENTRY_BATCH; j++) {
> +			cfg->sections[i]->entries[j] = malloc(sizeof(struct
> +							rte_cfgfile_entry));
> +			if (cfg->sections[i]->entries[j] == NULL)
> +				return NULL;
> +		}
> +		cfg->sections[i]->allocated_entries = CFG_ALLOC_ENTRY_BATCH;
> +		cfg->sections[i]->free_entries = CFG_ALLOC_ENTRY_BATCH;
> +	}
> +	cfg->allocated_sections = CFG_ALLOC_SECTION_BATCH;
> +	cfg->free_sections = CFG_ALLOC_SECTION_BATCH;

This work to initialise things is duplicated from the other add_section
and add_entries functions. I'd just remove it from here, and have the
newly allocated cfgfile structure be completely empty. Then on adding
the first section and first entry, the allocation happens. No need to do
it here.

> +
> +	if (flags & CFG_FLAG_GLOBAL_SECTION)
> +		rte_cfgfile_add_section(cfg, "GLOBAL");
> +	return cfg;
> +}
> +
> +int
> +rte_cfgfile_add_section(struct rte_cfgfile *cfg, const char *sectionname)
> +{
> +	int i;

You are missing parameter checking.

> +	/* resize overall struct if we don't have room for more	sections */
> +	if (cfg->free_sections == 0) {
> +
> +		struct rte_cfgfile_section **n_sections =
> +				realloc(cfg->sections,
> +				sizeof(cfg->sections[0]) *
> +				((cfg->allocated_sections) +
> +						CFG_ALLOC_SECTION_BATCH));
> +
> +		if (n_sections == NULL)
> +			return -ENOMEM;
> +
> +		cfg->sections = n_sections;
> +
> +		for (i = cfg->allocated_sections;
> +				i < (cfg->allocated_sections) +
> +						CFG_ALLOC_SECTION_BATCH; i++) {
> +			cfg->sections[i] =
> +				malloc(sizeof(struct rte_cfgfile_section));
> +
> +			if (cfg->sections[i] == NULL)
> +				return -ENOMEM;
> +
> +			memset(cfg->sections[i], 0,
> +					sizeof(struct rte_cfgfile_section));
> +		}

Same comment for too many mallocs (and memsets) as with add_entry.

> +		cfg->allocated_sections += CFG_ALLOC_SECTION_BATCH;
> +		cfg->free_sections += CFG_ALLOC_SECTION_BATCH;
> +	}
> +
> +	snprintf(cfg->sections[cfg->num_sections]->name,
> +			sizeof(cfg->sections[0]->name), "%s", sectionname);
> +	cfg->sections[cfg->num_sections]->num_entries = 0;
> +
> +	cfg->num_sections++;
> +	cfg->free_sections--;
> +
> +	return 0;
> +}
> +
> +int rte_cfgfile_add_entry(struct rte_cfgfile *cfg,
> +		const char *sectionname, const char *entryname,
> +		const char *entryvalue)
> +{
> +	int ret;
> +
> +	if (cfg == NULL)
> +		return -EINVAL;
> +
Check other parameters for NULL too?

> +	if (rte_cfgfile_has_entry(cfg, sectionname, entryname) != 0)
> +		return -EEXIST;
> +
> +	/* search for section pointer by sectionname */
> +	struct rte_cfgfile_section *curr_section = _get_section(cfg,
> +								sectionname);
> +	if (curr_section == NULL)
> +		return -EINVAL;
> +
> +	ret = _add_entry(curr_section, entryname, entryvalue);
> +
> +	return ret;
> +}
> +
> +int rte_cfgfile_set_entry(struct rte_cfgfile *cfg, const char *sectionname,
> +		const char *entryname, const char *entryvalue)
> +{
> +	int i;
> +
> +	if (cfg == NULL)
> +		return -EINVAL;
> +

parameter checking?

> +	/* search for section pointer by sectionname */
> +	struct rte_cfgfile_section *curr_section = _get_section(cfg,
> +								sectionname);
> +	if (curr_section == NULL)
> +		return -EINVAL;
> +
> +	if (entryvalue == NULL)
> +		entryvalue = "";
> +
> +	for (i = 0; i < curr_section->num_entries; i++)
> +		if (!strcmp(curr_section->entries[i]->name, entryname)) {
> +			strcpy(curr_section->entries[i]->value, entryvalue);

strcpy is unsafe here. Use snprintf as in other cases above where you
are setting string values.

> +			return 0;
> +		}
> +	return -1;
> +}
> +
> +int rte_cfgfile_save(struct rte_cfgfile *cfg, const char *filename)
> +{
> +	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
> +	int i, j;
> +
> +	if ((cfg == NULL) || (filename == NULL))
> +		return -EINVAL;
> +
> +	FILE *f = fopen(filename, "w");
> +
> +	if (f == NULL)
> +		return -EINVAL;

Other functions e.g. the one above return -1 on error (and should set
rte_errno appropriately in that case). Please standardize on one
approach for error handling in this library - either return -1 and set
rte_errno (my preference), or return error code directly. Mixing both is
confusing.

> +
> +	for (i = 0; i < cfg->num_sections; i++) {
> +		snprintf(buffer, sizeof(buffer), "[%s]\n",
> +					cfg->sections[i]->name);
> +		fputs(buffer, f);
> +
> +		for (j = 0; j < cfg->sections[i]->num_entries; j++) {
> +			snprintf(buffer, sizeof(buffer), "%s=%s\n",
> +				cfg->sections[i]->entries[j]->name,
> +				cfg->sections[i]->entries[j]->value);
> +			fputs(buffer, f);

Is there a reason to write to the buffer first and then to the file? Why
not just write directly to the file?
Check the return value from the write too.

> +		}
> +	}
> +	return fclose(f);
> +}
>  
>  int rte_cfgfile_close(struct rte_cfgfile *cfg)
>  {
> @@ -385,17 +619,6 @@ rte_cfgfile_sections(struct rte_cfgfile *cfg, char *sections[],
>  	return i;
>  }
>  
> -static const struct rte_cfgfile_section *
> -_get_section(struct rte_cfgfile *cfg, const char *sectionname)
> -{
> -	int i;
> -	for (i = 0; i < cfg->num_sections; i++) {
> -		if (strncmp(cfg->sections[i]->name, sectionname,
> -				sizeof(cfg->sections[0]->name)) == 0)
> -			return cfg->sections[i];
> -	}
> -	return NULL;
> -}
>  
>  int
>  rte_cfgfile_has_section(struct rte_cfgfile *cfg, const char *sectionname)
> diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
> index fa10d40..6245c7e 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.h
> +++ b/lib/librte_cfgfile/rte_cfgfile.h
> @@ -121,6 +121,82 @@ struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename,
>  	int flags, const struct rte_cfgfile_parameters *params);
>  
>  /**
> + * Create new cfgfile instance with empty sections and entries
> + *
> + * @param flags
> + *   - CFG_FLAG_GLOBAL_SECTION
> + *     Indicates that the file supports key value entries before the first
> + *     defined section.  These entries can be accessed in the "GLOBAL"
> + *     section.
> + *   - CFG_FLAG_EMPTY_VALUES
> + *     Indicates that file supports key value entries where the value can
> + *     be zero length (e.g., "key=").
> + * @return
> + *   Handle to cfgfile instance on success, NULL otherwise
> + */
> +struct rte_cfgfile *rte_cfgfile_create(int flags);
> +
> +/**
> + * Add section in cfgfile instance.
> + *
> + * @param cfg
> + *   Pointer to the cfgfile structure.
> + * @param sectionname
> + *   Section name which will be add to cfgfile.
> + * @return
> + *   0 on success, -ENOMEM if can't add section
> + */
> +int
> +rte_cfgfile_add_section(struct rte_cfgfile *cfg, const char *sectionname);
> +
> +/**
> + * Add entry to specified section in cfgfile instance.
> + *
> + * @param cfg
> + *   Pointer to the cfgfile structure.
> + * @param sectionname
> + *   Given section name to add an entry.
> + * @param entryname
> + *   Entry name to add.
> + * @entryvalue
> + *   Entry value to add.
> + * @return
> + *   0 on success, -EEXIST if entry already exist, -EINVAL if bad argument
> + */
> +int rte_cfgfile_add_entry(struct rte_cfgfile *cfg,
> +		const char *sectionname, const char *entryname,
> +		const char *entryvalue);
> +
> +/**
> + * Update value of specified entry name in given section in config file
> + *
> + * @param cfg
> + *   Config file
> + * @param sectionname
> + *   Section name
> + * @param entryname
> + *   Entry name to look for the value change
> + * @param entryvalue
> + *   New entry value. Can be also an empty string if CFG_FLAG_EMPTY_VALUES = 1
> + * @return
> + *   0 on success, -EINVAL if bad argument

What about if the value doesn't exist?

> + */
> +int rte_cfgfile_set_entry(struct rte_cfgfile *cfg, const char *sectionname,
> +		const char *entryname, const char *entryvalue);
> +
> +/**
> + * Save object cfgfile to file on disc
> + *
> + * @param cfg
> + *   Config file structure
> + * @param filename
> + *   File name to save data
> + * @return
> + *   0 on success, errno otherwise
> + */
> +int rte_cfgfile_save(struct rte_cfgfile *cfg, const char *filename);
> +
> +/**
>  * Get number of sections in config file
>  *
>  * @param cfg
> diff --git a/lib/librte_cfgfile/rte_cfgfile_version.map b/lib/librte_cfgfile/rte_cfgfile_version.map
> index 5fe60f7..de68ff6 100644
> --- a/lib/librte_cfgfile/rte_cfgfile_version.map
> +++ b/lib/librte_cfgfile/rte_cfgfile_version.map
> @@ -27,3 +27,14 @@ DPDK_17.05 {
>      rte_cfgfile_load_with_params;
>  
>  } DPDK_16.04;
> +
> +DPDK_17.08 {
> +	global:
> +
> +	rte_cfgfile_add_entry;
> +	rte_cfgfile_add_section;
> +	rte_cfgfile_create;
> +	rte_cfgfile_save;
> +	rte_cfgfile_set_entry;
> +
> +} DPDK_17.05;
> \ No newline at end of file
> -- 
> 2.7.4
>