From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 238432C18 for ; Fri, 9 Mar 2018 20:07:21 +0100 (CET) Received: from cpe-2606-a000-111b-40b7-640c-26a-4e16-9225.dyn6.twc.com ([2606:a000:111b:40b7:640c:26a:4e16:9225] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1euNMB-000293-Uo; Fri, 09 Mar 2018 14:07:14 -0500 Date: Fri, 9 Mar 2018 14:06:35 -0500 From: Neil Horman To: Ferruh Yigit Cc: John McNamara , Marko Kovacevic , Thomas Monjalon , dev@dpdk.org Message-ID: <20180309190635.GD19004@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> <20180309151616.GC19004@hmswarspite.think-freely.org> <965f5230-a650-e848-3668-0c471d3a2817@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <965f5230-a650-e848-3668-0c471d3a2817@intel.com> User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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 19:07:21 -0000 On Fri, Mar 09, 2018 at 03:45:49PM +0000, Ferruh Yigit wrote: > 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 > Well...yes. Thats what I said > 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. > I'm sorry, thats not ok with me. Just declaring a data structure to be internal-only without enforcing that is asking for applications to mangle internal data, and theres no reason it can't be fixed (and done without sacrificing performance). > 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 *". > Just typedef a void pointer to some rte_ethdev_cb_handle_t type and pass that back and forth instead. That at least hides the fact that you are using a non opaque structure from user applications without some intentional casting. You can further lock the call down by declaring the handles const so that no one tries to dereference or modify them without generating a warning. Neil > > > > Neil > > > >>> > >>> Neil > >>> > >> > >> > >