From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id CCD1834F0 for ; Tue, 20 Mar 2018 16:55:45 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Mar 2018 08:51:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,336,1517904000"; d="scan'208";a="35378296" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.63]) ([10.237.221.63]) by FMSMGA003.fm.intel.com with ESMTP; 20 Mar 2018 08:51:54 -0700 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> <965f5230-a650-e848-3668-0c471d3a2817@intel.com> <20180309190635.GD19004@hmswarspite.think-freely.org> From: Ferruh Yigit Message-ID: <899e769e-4d43-d510-ca2c-661b19d47e97@intel.com> Date: Tue, 20 Mar 2018 15:51:53 +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: <20180309190635.GD19004@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: Tue, 20 Mar 2018 15:55:46 -0000 On 3/9/2018 7:06 PM, Neil Horman wrote: > 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). Currently that is what blocking us, mainly rte_eth_[rt]x_burst() inline functions. Is there a way to fix it without loosing 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. Even handle won't work if user really wants to update it. This may highlight the intention but I believe moving struct to ethdev_core.h already highlights the intention. Related to the const qualifier I was thinking if remove functions needs to update the struct, but it doesn't seems the case, I will add them. > > Neil > >>> >>> Neil >>> >>>>> >>>>> Neil >>>>> >>>> >>>> >> >>