From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 8C7E5377A for ; Fri, 30 Jun 2017 12:28:44 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jun 2017 03:28:43 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,286,1496127600"; d="scan'208";a="120783042" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.28]) by fmsmga005.fm.intel.com with SMTP; 30 Jun 2017 03:28:41 -0700 Received: by (sSMTP sendmail emulation); Fri, 30 Jun 2017 11:28:41 +0100 Date: Fri, 30 Jun 2017 11:28:40 +0100 From: Bruce Richardson To: Jacek Piasecki Cc: dev@dpdk.org, deepak.k.jain@intel.com Message-ID: <20170630102840.GC14776@bricha3-MOBL3.ger.corp.intel.com> References: <1498474759-102089-2-git-send-email-jacekx.piasecki@intel.com> <1498559210-104084-1-git-send-email-jacekx.piasecki@intel.com> <1498559210-104084-3-git-send-email-jacekx.piasecki@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1498559210-104084-3-git-send-email-jacekx.piasecki@intel.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.1 (2017-04-11) Subject: Re: [dpdk-dev] [PATCH v3 2/4] cfgfile: add new functions to API X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Jun 2017 10:28:45 -0000 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 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 > #include > #include > +#include > #include > > #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 >