Re: [dpdk-dev] [PATCH v3 3/4] cfgfile: rework of load function

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Jun 27, 2017 at 12:26:49PM +0200, Jacek Piasecki wrote:
> From: Kuba Kozak <kubax.kozak@intel.com>
> 
> New functions added to cfgfile library make it possible
> to significantly simplify the code of rte_cfgfile_load_with_params()
> 
> This patch shows the new body of this function.
> 
> Signed-off-by: Jacek Piasecki <jacekx.piasecki@intel.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 143 +++++----------------------------------
>  1 file changed, 17 insertions(+), 126 deletions(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 518b6ab..5625c80 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -207,10 +207,6 @@ struct rte_cfgfile *
>  rte_cfgfile_load_with_params(const char *filename, int flags,
>  			     const struct rte_cfgfile_parameters *params)
>  {
> -	int allocated_sections = CFG_ALLOC_SECTION_BATCH;
> -	int allocated_entries = 0;
> -	int curr_section = -1;
> -	int curr_entry = -1;
>  	char buffer[CFG_NAME_LEN + CFG_VALUE_LEN + 4] = {0};
>  	int lineno = 0;
>  	struct rte_cfgfile *cfg = NULL;
> @@ -222,28 +218,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  	if (f == NULL)
>  		return NULL;
>  
> -	cfg = malloc(sizeof(*cfg) + sizeof(cfg->sections[0]) *
> -		allocated_sections);
> -	if (cfg == NULL)
> -		goto error2;
> -
> -	memset(cfg->sections, 0, sizeof(cfg->sections[0]) * allocated_sections);
> -
> -	if (flags & CFG_FLAG_GLOBAL_SECTION) {
> -		curr_section = 0;
> -		allocated_entries = CFG_ALLOC_ENTRY_BATCH;
> -		cfg->sections[curr_section] = malloc(
> -			sizeof(*cfg->sections[0]) +
> -			sizeof(cfg->sections[0]->entries[0]) *
> -			allocated_entries);
> -		if (cfg->sections[curr_section] == NULL) {
> -			printf("Error - no memory for global section\n");
> -			goto error1;
> -		}
> -
> -		snprintf(cfg->sections[curr_section]->name,
> -				 sizeof(cfg->sections[0]->name), "GLOBAL");
> -	}
> +	cfg = rte_cfgfile_create(flags);
>  
>  	while (fgets(buffer, sizeof(buffer), f) != NULL) {
>  		char *pos = NULL;
> @@ -254,6 +229,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  					"Check if line too long\n", lineno);
>  			goto error1;
>  		}
> +		/* skip parsing if comment character found */
>  		pos = memchr(buffer, params->comment_character, len);
>  		if (pos != NULL) {
>  			*pos = '\0';
> @@ -261,6 +237,7 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  		}
>  
>  		len = _strip(buffer, len);
> +		/* skip lines without useful content */
>  		if (buffer[0] != '[' && memchr(buffer, '=', len) == NULL)
>  			continue;
>  
> @@ -268,130 +245,44 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  			/* section heading line */
>  			char *end = memchr(buffer, ']', len);
>  			if (end == NULL) {
> -				printf("Error line %d - no terminating '['"
> +				printf("Error line %d - no terminating ']'"
>  					"character found\n", lineno);
>  				goto error1;
>  			}
>  			*end = '\0';
>  			_strip(&buffer[1], end - &buffer[1]);
>  
> -			/* close off old section and add start new one */
> -			if (curr_section >= 0)
> -				cfg->sections[curr_section]->num_entries =
> -					curr_entry + 1;
> -			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) {
> -					curr_section--;
> -					printf("Error - no more memory\n");
> -					goto error1;
> -				}
> -				cfg = n_cfg;
> -			}
> -
> -			/* allocate space for new section */
> -			allocated_entries = CFG_ALLOC_ENTRY_BATCH;
> -			curr_entry = -1;
> -			cfg->sections[curr_section] = malloc(
> -				sizeof(*cfg->sections[0]) +
> -				sizeof(cfg->sections[0]->entries[0]) *
> -				allocated_entries);
> -			if (cfg->sections[curr_section] == NULL) {
> -				printf("Error - no more memory\n");
> -				goto error1;
> -			}
> -
> -			snprintf(cfg->sections[curr_section]->name,
> -					sizeof(cfg->sections[0]->name),
> -					"%s", &buffer[1]);
> +			rte_cfgfile_add_section(cfg, &buffer[1]);
>  		} else {
> -			/* value line */
> -			if (curr_section < 0) {
> -				printf("Error line %d - value outside of"
> -					"section\n", lineno);
> -				goto error1;
> -			}
> -
> -			struct rte_cfgfile_section *sect =
> -				cfg->sections[curr_section];
> -
> +			/* key and value line */
>  			char *split[2] = {NULL};
> +
>  			split[0] = buffer;
>  			split[1] = memchr(buffer, '=', len);
> +			*split[1] = '\0';
> +			split[1]++;
> +
> +			_strip(split[0], strlen(split[0]));
> +			_strip(split[1], strlen(split[1]));
>  
> -			/* when delimeter not found */
> -			if (split[1] == NULL) {
> +			if (!(flags & CFG_FLAG_EMPTY_VALUES) &&
> +					(*split[1] == '\0')) {
>  				printf("Error at line %d - cannot "
>  					"split string\n", lineno);

This error message could do with an update. It's not that you can't
split the string, it's that the value is empty - something not allowed.
Also, it's bad practice to split literal strings for printing. This is
the one case where you are encouraged to go over the 80-char limit
rather than wrapping the line.

/Bruce