From mboxrd@z Thu Jan 1 00:00:00 1970 From: Neil Horman Subject: Re: [PATCH v4] ethdev: return named opaque type instead of void pointer Date: Fri, 9 Mar 2018 10:16:16 -0500 Message-ID: <20180309151616.GC19004@hmswarspite.think-freely.org> References: <20180117215802.90809-2-ferruh.yigit@intel.com> <20180309112531.292163-1-ferruh.yigit@intel.com> <20180309123651.GB19004@hmswarspite.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: John McNamara , Marko Kovacevic , Thomas Monjalon , dev@dpdk.org To: Ferruh Yigit Return-path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 6E63F5F17 for ; Fri, 9 Mar 2018 16:17:11 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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. Neil > > > > Neil > > > >