From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 6F8BE58CE for ; Wed, 31 May 2017 16:20:06 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 May 2017 07:20:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.38,423,1491289200"; d="scan'208";a="1176547100" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.42]) by fmsmga002.fm.intel.com with SMTP; 31 May 2017 07:20:01 -0700 Received: by (sSMTP sendmail emulation); Wed, 31 May 2017 15:20:00 +0100 Date: Wed, 31 May 2017 15:20:00 +0100 From: Bruce Richardson To: Jacek Piasecki Cc: dev@dpdk.org, deepak.k.jain@intel.com, michalx.k.jastrzebski@intel.com Message-ID: <20170531141959.GA37544@bricha3-MOBL3.ger.corp.intel.com> References: <1496133037-3014-1-git-send-email-jacekx.piasecki@intel.com> <1496133037-3014-2-git-send-email-jacekx.piasecki@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1496133037-3014-2-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 1/3] cfgfile: add new API functions 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: Wed, 31 May 2017 14:20:07 -0000 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 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(-) > > > +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. > +} >