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 4D69D591A for ; Wed, 27 May 2015 17:36:36 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 27 May 2015 08:36:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,506,1427785200"; d="scan'208";a="701056139" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.220.149]) by orsmga001.jf.intel.com with SMTP; 27 May 2015 08:36:33 -0700 Received: by (sSMTP sendmail emulation); Wed, 27 May 2015 16:36:32 +0025 Date: Wed, 27 May 2015 16:36:32 +0100 From: Bruce Richardson To: Marc Sune Message-ID: <20150527153632.GC10480@bricha3-MOBL3> References: <1432734469-1668-1-git-send-email-bruce.richardson@intel.com> <5565CC22.8050003@bisdn.de> <20150527135533.GB10480@bricha3-MOBL3> <5565D195.9040701@bisdn.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5565D195.9040701@bisdn.de> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 1/4] kni: add function to query the name of a kni object X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 May 2015 15:36:37 -0000 On Wed, May 27, 2015 at 04:15:49PM +0200, Marc Sune wrote: > > > On 27/05/15 15:55, Bruce Richardson wrote: > >On Wed, May 27, 2015 at 03:52:34PM +0200, Marc Sune wrote: > >> > >>On 27/05/15 15:47, Bruce Richardson wrote: > >>>When a KNI object is created, a name is assigned to it which is stored > >>>internally. There is also an API function to look up a KNI object by > >>>name, but there is no API to query the current name of an existing > >>>KNI object. This patch adds just such an API. > >>> > >>>Signed-off-by: Bruce Richardson > >>>--- > >>> lib/librte_kni/rte_kni.c | 6 ++++++ > >>> lib/librte_kni/rte_kni.h | 10 ++++++++++ > >>> lib/librte_kni/rte_kni_version.map | 1 + > >>> 3 files changed, 17 insertions(+) > >>> > >>>diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c > >>>index 4e70fa0..c5a0089 100644 > >>>--- a/lib/librte_kni/rte_kni.c > >>>+++ b/lib/librte_kni/rte_kni.c > >>>@@ -674,6 +674,12 @@ rte_kni_get(const char *name) > >>> return NULL; > >>> } > >>>+const char * > >>>+rte_kni_get_name(const struct rte_kni *kni) > >>>+{ > >>>+ return kni->name; > >>>+} > >>Since a pointer to the kni context (struct rte_kni) is exposed to the user > >>(rte_kni_get() and rte_kni_alloc ()), and the field is directly in the > >>struct, is this API call really necessary? I would only see this necessary > >>if the API would only expose a handle, like a port_id for ethdev > >> > >>Marc > >The structure definition is in rte_kni.c, not in the header file, so applications > >can't read the name directly. In other words, the create API just exposes a handle. > >[The structure in the header is the conf structure, not the full kni struct] > > Ops, you are right. I overlooked that. What about: > > extern void rte_kni_get_config(const struct rte_kni *kni, struct > rte_kni_conf* conf); > > which fills in (copies) the fields of conf would allow to recover the > original configuration, including the name? It is closer > rte_eth_dev_info_get (unfortunately rte_kni_info_get is taken by the > deprecated API), and would work if we add more params to rte_kni_conf. > > Thanks > marc > Given the issues that have been flagged recently around ABI compatibility, I don't think I'd introduce such an API. If provided like you describe, it makes the calling application very dependent upon the size and structure of the conf structure. Having specific function like this to return specific values is safer that way. An alternative is to have a function which returns the conf structure as a return value, rather than as an in-out arg. That would allow us to add fields to the end of the structure without breaking applications using that particular function. This would require us to store the entire conf structure inside the rte_kni structure though, so we can return a const pointer to it. This is a bit more invasive of a change. Overall, I prefer the query-single value option, as I think it's generally the best-practice for working with structures going forward. [Neil, please correct me if I'm wrong here!]. If however, you see a current need for looking at the other fields inside the KNI, I can see about changing things to return the whole config structure as a const pointer. /Bruce