From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 5D7554D27 for ; Fri, 9 Mar 2018 16:45:53 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Mar 2018 07:45:51 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,446,1515484800"; d="scan'208";a="181344369" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.62]) ([10.237.221.62]) by orsmga004.jf.intel.com with ESMTP; 09 Mar 2018 07:45:49 -0800 To: Neil Horman Cc: John McNamara , Marko Kovacevic , Thomas Monjalon , dev@dpdk.org References: <20180117215802.90809-2-ferruh.yigit@intel.com> <20180309112531.292163-1-ferruh.yigit@intel.com> <20180309123651.GB19004@hmswarspite.think-freely.org> <20180309151616.GC19004@hmswarspite.think-freely.org> From: Ferruh Yigit Message-ID: <965f5230-a650-e848-3668-0c471d3a2817@intel.com> Date: Fri, 9 Mar 2018 15:45:49 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180309151616.GC19004@hmswarspite.think-freely.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4] ethdev: return named opaque type instead of void pointer 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, 09 Mar 2018 15:45:53 -0000 On 3/9/2018 3:16 PM, Neil Horman wrote: > On Fri, Mar 09, 2018 at 01:00:35PM +0000, Ferruh Yigit wrote: >> On 3/9/2018 12:36 PM, Neil Horman wrote: >>> On Fri, Mar 09, 2018 at 11:25:31AM +0000, Ferruh Yigit wrote: >>>> "struct rte_eth_rxtx_callback" is defined as internal data structure and >>>> used as named opaque type. >>>> >>>> So the functions that are adding callbacks can return objects in this >>>> type instead of void pointer. >>>> >>>> Signed-off-by: Ferruh Yigit >>>> Acked-by: Stephen Hemminger >>>> --- >>>> v2: >>>> * keep using struct * in parameters, instead add callback functions >>>> return struct rte_eth_rxtx_callback pointer. >>>> >>>> v4: >>>> * Remove deprecation notice. LIBABIVER already increased in this release >>>> --- >>>> doc/guides/rel_notes/deprecation.rst | 7 ------- >>>> lib/librte_ether/rte_ethdev.c | 6 +++--- >>>> lib/librte_ether/rte_ethdev.h | 13 ++++++++----- >>>> 3 files changed, 11 insertions(+), 15 deletions(-) >>>> >>> This doesn't quite make sense to me. If rte_eth_rxtx_callback is defined as an >>> internal data structure, then it shouldn't be used as part of the prototype for >>> an exported function, as the structure will then no longer be a internal data >>> structure, but rather part of the public ABI. >> >> "struct rte_eth_rxtx_callback" is internal data structure. And application >> should not access elements of this structure. >> >> "struct rte_eth_rxtx_callback;" is defined in the public header, so applications >> can use it as opaque type. >> >> It is possible that both "add" and "remove" APIs use "void *" and API itself can >> cast it. But the inconsistency was "add" related APIs return "void *" and >> "remove" related APIs require a parameter in "struct rte_eth_rxtx_callback *" type. >> >> While unifying the usage, "struct rte_eth_rxtx_callback *" preferred against >> "void *", because named opaque type documents intention/usage better. >> >> Thanks, >> ferruh >> > I get what you're saying about rte_eth_rxtx_callback being an internals > structure (or its intent is to be an internal structure), but it doesn't seem to > hold up to the header file layout. rte_eth_rxtx_callback is defined in > rte_ethdev_core.h which according to the makefile, is listed as a symlinked > file, and therefore available for external applications to include. This > negates the intended opaque nature of the struct. I think before you do this, > you want to rectify that. Intention is to make "struct rte_eth_rxtx_callback" internal, but as you said it is available to applications. This is same for all data structures in rte_ethdev_core.h Unfortunately it can't be actual internal because of inline functions in public header uses them. And we can't change inline functions because of performance concerns. Since we can't make the structure real internal, we can't really prevent applications to access the internals, this same if you use "void *". > > Neil > >>> >>> Neil >>> >> >>