From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <nhorman@tuxdriver.com>
Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58])
 by dpdk.org (Postfix) with ESMTP id 6E63F5F17
 for <dev@dpdk.org>; Fri,  9 Mar 2018 16:17:11 +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 <nhorman@tuxdriver.com>)
 id 1euJlJ-0008VU-UC; Fri, 09 Mar 2018 10:17:01 -0500
Date: Fri, 9 Mar 2018 10:16:16 -0500
From: Neil Horman <nhorman@tuxdriver.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: John McNamara <john.mcnamara@intel.com>,
 Marko Kovacevic <marko.kovacevic@intel.com>,
 Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org
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>
 <b12988d0-cb4a-2dcb-c20a-37cfbdf73eda@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b12988d0-cb4a-2dcb-c20a-37cfbdf73eda@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 <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 09 Mar 2018 15:17:11 -0000

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 <ferruh.yigit@intel.com>
> >> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> >> ---
> >> 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
> > 
> 
>