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 088D836E for ; Fri, 3 Mar 2017 13:10:24 +0100 (CET) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Mar 2017 04:10:23 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,236,1484035200"; d="scan'208";a="72369808" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.61]) by fmsmga006.fm.intel.com with SMTP; 03 Mar 2017 04:10:21 -0800 Received: by (sSMTP sendmail emulation); Fri, 03 Mar 2017 12:10:20 +0000 Date: Fri, 3 Mar 2017 12:10:20 +0000 From: Bruce Richardson To: "Legacy, Allain" Cc: "Dumitrescu, Cristian" , Yuanhan Liu , "dev@dpdk.org" , "Jolliffe, Ian (Wind River)" Message-ID: <20170303121020.GA193932@bricha3-MOBL3.ger.corp.intel.com> References: <1488482971-170522-1-git-send-email-allain.legacy@windriver.com> <1488482971-170522-2-git-send-email-allain.legacy@windriver.com> <20170302211015.GA18940@bricha3-MOBL3.ger.corp.intel.com> <20170303005337.GB18844@yliu-dev.sh.intel.com> <3EB4FA525960D640B5BDFFD6A3D8912652758102@IRSMSX108.ger.corp.intel.com> <70A7408C6E1BFB41B192A929744D85238A75B22B@ALA-MBC.corp.ad.wrs.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <70A7408C6E1BFB41B192A929744D85238A75B22B@ALA-MBC.corp.ad.wrs.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.0 (2017-02-23) Subject: Re: [dpdk-dev] [PATCH 1/5] cfgfile: configurable comment character 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, 03 Mar 2017 12:10:25 -0000 On Fri, Mar 03, 2017 at 11:31:11AM +0000, Legacy, Allain wrote: > > -----Original Message----- > > From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com] > > Possible options that I see: > > 1. Add a new parameters argument to the load functions (e.g. struct > > cfgfile_params *p), whit the comment char as one (and currently only) field > > of this struct. Drawbacks: API change that might have to be announced one > > release before the actual API change. > > I would prefer this option as it provides more flexibility. We can leave the existing API as is and a wrapper that accepts additional parameters. Something like the following (with implementations in the .c obviously rather than inline the header like I have it here). There are several examples of this pattern already in the dpdk (i.e., ring APIs, mempool APIs, etc.) where we use a common function invoked by higher level functions that pass in additional parameters to customize behavior. > > struct rte_cfgfile *_rte_cfgfile_load(const char *filename, > const struct rte_cfgfile_params *params); > > struct rte_cfgfile *rte_cfgfile_load(const char *filename, int flags) > { > struct rte_cfgfile_params params; > > rte_cfgfile_set_default_params(¶ms); > params |= flags; > return _rte_cfgfile_load(filename, ¶ms); > } > > struct rte_cfgfile *rte_cfgfile_load_with_params(const char *filename, > const struct rte_cfgfile_params *params) > { > return _rte_cfgfile_load(filename, params); > } No need for a new API. Just add the extra parameter to the existing load parameter and use function versioning for ABI compatilibity. Since it's only one function, I don't think using versioning is a big deal, and that's what it is there for. Also, for a single parameter like a comment char, I don't think we need to go creating a separate structure. The current flags parameter is unused, so just replace it with the comment char one. With using the structure, any additions to the struct would be an ABI change anyway, so I see little point in using it, unless we already know of additional parameters we will be adding in future. [It's an ABI change even when adding to the end, since the struct is allocated in the app itself, not the library.] /Bruce