diff mbox

[dpdk-dev,v5,2/5] cfgfile: change existing API functions

Message ID 1505819248-19792-1-git-send-email-kubax.kozak@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Kuba Kozak Sept. 19, 2017, 11:07 a.m. UTC 
  From: Jacek Piasecki <jacekx.piasecki@intel.com>

Change to flat arrays in cfgfile struct force slightly
diffrent data access for most of cfgfile functions.
This patch provides necessary changes in existing API.

Signed-off-by: Jacek Piasecki <jacekx.piasecki@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/rel_notes/deprecation.rst |   5 ++
 lib/librte_cfgfile/rte_cfgfile.c     | 133 ++++++++++++++++++-----------------
 lib/librte_cfgfile/rte_cfgfile.h     |   6 +-
 3 files changed, 78 insertions(+), 66 deletions(-)

Comments

Thomas Monjalon Sept. 19, 2017, 9:26 p.m. UTC | #1 
Hi,

19/09/2017 13:07, Kuba Kozak:
> @@ -409,7 +407,11 @@ rte_cfgfile_section_num_entries(struct rte_cfgfile *cfg,
>  {
>  	const struct rte_cfgfile_section *s = _get_section(cfg, sectionname);
>  	if (s == NULL)
> +#ifdef RTE_NEXT_ABI
> +		return -EINVAL;
> +#else
>  		return -1;
> +#endif
>  	return s->num_entries;
>  }

Why are you using RTE_NEXT_ABI?
Can you wait 18.02 to make this change?

Anyway, when breaking the API you need to update tha API section
of the release notes.

> @@ -219,7 +219,7 @@ int rte_cfgfile_section_num_entries_by_index(struct rte_cfgfile *cfg,
>  * @param max_entries
>  *   Maximum number of section entries to be stored in entries array
>  * @return
> -*   Number of entries populated on success, -1 otherwise
> +*   Number of entries populated on success, -EINVAL otherwise
>  */

This documentation become wrong if RTE_NEXT_ABI is disabled.
Kuba Kozak Sept. 21, 2017, 9:12 a.m. UTC | #2 
Hi Thomas,

Yes we can wait to 18.02. I'll prepare v6 patchset without these changes.

Best regards,
Kuba

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, September 19, 2017 23:27
> To: Kozak, KubaX <kubax.kozak@intel.com>
> Cc: dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Jain, Deepak K
> <deepak.k.jain@intel.com>; Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>; Piasecki,
> JacekX <jacekx.piasecki@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] cfgfile: change existing API functions
> 
> Hi,
> 
> 19/09/2017 13:07, Kuba Kozak:
> > @@ -409,7 +407,11 @@ rte_cfgfile_section_num_entries(struct
> > rte_cfgfile *cfg,  {
> >  	const struct rte_cfgfile_section *s = _get_section(cfg, sectionname);
> >  	if (s == NULL)
> > +#ifdef RTE_NEXT_ABI
> > +		return -EINVAL;
> > +#else
> >  		return -1;
> > +#endif
> >  	return s->num_entries;
> >  }
> 
> Why are you using RTE_NEXT_ABI?
> Can you wait 18.02 to make this change?
> 
> Anyway, when breaking the API you need to update tha API section of the release notes.
> 
> > @@ -219,7 +219,7 @@ int
> > rte_cfgfile_section_num_entries_by_index(struct rte_cfgfile *cfg,
> >  * @param max_entries
> >  *   Maximum number of section entries to be stored in entries array
> >  * @return
> > -*   Number of entries populated on success, -1 otherwise
> > +*   Number of entries populated on success, -EINVAL otherwise
> >  */
> 
> This documentation become wrong if RTE_NEXT_ABI is disabled.
diff mbox

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 3362f33..4f67646 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -120,3 +120,8 @@  Deprecation Notices
   The non-"do-sig" versions of the hash tables will be removed
   (including the ``signature_offset`` parameter)
   and the "do-sig" versions renamed accordingly.
+
+* cfgfile: Return value in ``rte_cfgfile_section_num_entries``,
+  ``rte_cfgfile_section_entries`` and ``rte_cfgfile_section_entries_by_index``
+  will change from ``-1`` to ``-EINVAL`` to get a consistent error
+  handling in whole cfgfile library.
diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 9dc25cc..d7b3ff6 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,15 @@ 
 struct rte_cfgfile_section {
 	char name[CFG_NAME_LEN];
 	int num_entries;
-	struct rte_cfgfile_entry *entries[0];
+	int allocated_entries;
+	struct rte_cfgfile_entry *entries;
 };
 
 struct rte_cfgfile {
 	int flags;
 	int num_sections;
-	struct rte_cfgfile_section *sections[0];
+	int allocated_sections;
+	struct rte_cfgfile_section *sections;
 };
 
 /** when we resize a file structure, how many extra entries
@@ -104,6 +107,19 @@  _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
 rte_cfgfile_check_params(const struct rte_cfgfile_parameters *params)
 {
@@ -168,17 +184,17 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 	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]) *
+		cfg->sections = malloc(
+			sizeof(cfg->sections[0]) +
+			sizeof(cfg->sections[0].entries) *
 			allocated_entries);
-		if (cfg->sections[curr_section] == NULL) {
+		if (cfg->sections == NULL) {
 			printf("Error - no memory for global section\n");
 			goto error1;
 		}
 
-		snprintf(cfg->sections[curr_section]->name,
-				 sizeof(cfg->sections[0]->name), "GLOBAL");
+		snprintf(cfg->sections[curr_section].name,
+				 sizeof(cfg->sections[0].name), "GLOBAL");
 	}
 
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
@@ -213,7 +229,7 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 
 			/* close off old section and add start new one */
 			if (curr_section >= 0)
-				cfg->sections[curr_section]->num_entries =
+				cfg->sections[curr_section].num_entries =
 					curr_entry + 1;
 			curr_section++;
 
@@ -235,17 +251,17 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 			/* 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]) *
+			cfg->sections = malloc(
+				sizeof(cfg->sections[0]) +
+				sizeof(cfg->sections[0].entries) *
 				allocated_entries);
-			if (cfg->sections[curr_section] == NULL) {
+			if (cfg->sections == NULL) {
 				printf("Error - no more memory\n");
 				goto error1;
 			}
 
-			snprintf(cfg->sections[curr_section]->name,
-					sizeof(cfg->sections[0]->name),
+			snprintf(cfg->sections[curr_section].name,
+					sizeof(cfg->sections[0].name),
 					"%s", &buffer[1]);
 		} else {
 			/* value line */
@@ -255,8 +271,7 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 				goto error1;
 			}
 
-			struct rte_cfgfile_section *sect =
-				cfg->sections[curr_section];
+			struct rte_cfgfile_section *sect = cfg->sections;
 
 			char *split[2] = {NULL};
 			split[0] = buffer;
@@ -292,18 +307,17 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 					printf("Error - no more memory\n");
 					goto error1;
 				}
-				sect = cfg->sections[curr_section] = n_sect;
+				sect = cfg->sections = n_sect;
 			}
 
-			sect->entries[curr_entry] = malloc(
-				sizeof(*sect->entries[0]));
-			if (sect->entries[curr_entry] == NULL) {
+			sect->entries = malloc(
+				sizeof(sect->entries[0]));
+			if (sect->entries == NULL) {
 				printf("Error - no more memory\n");
 				goto error1;
 			}
 
-			struct rte_cfgfile_entry *entry = sect->entries[
-				curr_entry];
+			struct rte_cfgfile_entry *entry = sect->entries;
 			snprintf(entry->name, sizeof(entry->name), "%s",
 				split[0]);
 			snprintf(entry->value, sizeof(entry->value), "%s",
@@ -319,42 +333,38 @@  rte_cfgfile_load_with_params(const char *filename, int flags,
 	cfg->num_sections = curr_section + 1;
 	/* curr_section will still be -1 if we have an empty file */
 	if (curr_section >= 0)
-		cfg->sections[curr_section]->num_entries = curr_entry + 1;
+		cfg->sections[curr_section].num_entries = curr_entry + 1;
 	return cfg;
 
 error1:
 	cfg->num_sections = curr_section + 1;
 	if (curr_section >= 0)
-		cfg->sections[curr_section]->num_entries = curr_entry + 1;
+		cfg->sections[curr_section].num_entries = curr_entry + 1;
 	rte_cfgfile_close(cfg);
 error2:
 	fclose(f);
 	return NULL;
 }
 
-
 int rte_cfgfile_close(struct rte_cfgfile *cfg)
 {
-	int i, j;
+	int i;
 
 	if (cfg == NULL)
 		return -1;
 
-	for (i = 0; i < cfg->num_sections; i++) {
-		if (cfg->sections[i] != NULL) {
-			if (cfg->sections[i]->num_entries) {
-				for (j = 0; j < cfg->sections[i]->num_entries;
-					j++) {
-					if (cfg->sections[i]->entries[j] !=
-						NULL)
-						free(cfg->sections[i]->
-							entries[j]);
-				}
+	if (cfg->sections != NULL) {
+		for (i = 0; i < cfg->allocated_sections; i++) {
+			if (cfg->sections[i].entries != NULL) {
+				free(cfg->sections[i].entries);
+				cfg->sections[i].entries = NULL;
 			}
-			free(cfg->sections[i]);
 		}
+		free(cfg->sections);
+		cfg->sections = NULL;
 	}
 	free(cfg);
+	cfg = NULL;
 
 	return 0;
 }
@@ -366,7 +376,7 @@  size_t length)
 	int i;
 	int num_sections = 0;
 	for (i = 0; i < cfg->num_sections; i++) {
-		if (strncmp(cfg->sections[i]->name, sectionname, length) == 0)
+		if (strncmp(cfg->sections[i].name, sectionname, length) == 0)
 			num_sections++;
 	}
 	return num_sections;
@@ -380,23 +390,11 @@  rte_cfgfile_sections(struct rte_cfgfile *cfg, char *sections[],
 
 	for (i = 0; i < cfg->num_sections && i < max_sections; i++)
 		snprintf(sections[i], CFG_NAME_LEN, "%s",
-		cfg->sections[i]->name);
+		cfg->sections[i].name);
 
 	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)
 {
@@ -409,7 +407,11 @@  rte_cfgfile_section_num_entries(struct rte_cfgfile *cfg,
 {
 	const struct rte_cfgfile_section *s = _get_section(cfg, sectionname);
 	if (s == NULL)
+#ifdef RTE_NEXT_ABI
+		return -EINVAL;
+#else
 		return -1;
+#endif
 	return s->num_entries;
 }
 
@@ -417,14 +419,12 @@  int
 rte_cfgfile_section_num_entries_by_index(struct rte_cfgfile *cfg,
 	char *sectionname, int index)
 {
-	const struct rte_cfgfile_section *sect;
-
 	if (index < 0 || index >= cfg->num_sections)
 		return -1;
 
-	sect = cfg->sections[index];
-	snprintf(sectionname, CFG_NAME_LEN, "%s", sect->name);
+	const struct rte_cfgfile_section *sect = &(cfg->sections[index]);
 
+	snprintf(sectionname, CFG_NAME_LEN, "%s", sect->name);
 	return sect->num_entries;
 }
 int
@@ -434,9 +434,13 @@  rte_cfgfile_section_entries(struct rte_cfgfile *cfg, const char *sectionname,
 	int i;
 	const struct rte_cfgfile_section *sect = _get_section(cfg, sectionname);
 	if (sect == NULL)
+#ifdef RTE_NEXT_ABI
+		return -EINVAL;
+#else
 		return -1;
+#endif
 	for (i = 0; i < max_entries && i < sect->num_entries; i++)
-		entries[i] = *sect->entries[i];
+		entries[i] = sect->entries[i];
 	return i;
 }
 
@@ -449,12 +453,15 @@  rte_cfgfile_section_entries_by_index(struct rte_cfgfile *cfg, int index,
 	const struct rte_cfgfile_section *sect;
 
 	if (index < 0 || index >= cfg->num_sections)
+#ifdef RTE_NEXT_ABI
+		return -EINVAL;
+#else
 		return -1;
-
-	sect = cfg->sections[index];
+#endif
+	sect = &cfg->sections[index];
 	snprintf(sectionname, CFG_NAME_LEN, "%s", sect->name);
 	for (i = 0; i < max_entries && i < sect->num_entries; i++)
-		entries[i] = *sect->entries[i];
+		entries[i] = sect->entries[i];
 	return i;
 }
 
@@ -467,9 +474,9 @@  rte_cfgfile_get_entry(struct rte_cfgfile *cfg, const char *sectionname,
 	if (sect == NULL)
 		return NULL;
 	for (i = 0; i < sect->num_entries; i++)
-		if (strncmp(sect->entries[i]->name, entryname, CFG_NAME_LEN)
-			== 0)
-			return sect->entries[i]->value;
+		if (strncmp(sect->entries[i].name, entryname, CFG_NAME_LEN)
+									== 0)
+			return sect->entries[i].value;
 	return NULL;
 }
 
diff --git a/lib/librte_cfgfile/rte_cfgfile.h b/lib/librte_cfgfile/rte_cfgfile.h
index 35dc419..112f57c 100644
--- a/lib/librte_cfgfile/rte_cfgfile.h
+++ b/lib/librte_cfgfile/rte_cfgfile.h
@@ -178,7 +178,7 @@  int rte_cfgfile_has_section(struct rte_cfgfile *cfg, const char *sectionname);
 * @param sectionname
 *   Section name
 * @return
-*   Number of entries in section on success, -1 otherwise
+*   Number of entries in section on success, -EINVAL otherwise
 */
 int rte_cfgfile_section_num_entries(struct rte_cfgfile *cfg,
 	const char *sectionname);
@@ -219,7 +219,7 @@  int rte_cfgfile_section_num_entries_by_index(struct rte_cfgfile *cfg,
 * @param max_entries
 *   Maximum number of section entries to be stored in entries array
 * @return
-*   Number of entries populated on success, -1 otherwise
+*   Number of entries populated on success, -EINVAL otherwise
 */
 int rte_cfgfile_section_entries(struct rte_cfgfile *cfg,
 	const char *sectionname,
@@ -246,7 +246,7 @@  int rte_cfgfile_section_entries(struct rte_cfgfile *cfg,
 * @param max_entries
 *   Maximum number of section entries to be stored in entries array
 * @return
-*   Number of entries populated on success, -1 otherwise
+*   Number of entries populated on success, -EINVAL otherwise
 */
 int rte_cfgfile_section_entries_by_index(struct rte_cfgfile *cfg,
 	int index,