[dpdk-dev] [PATCH v4] ethdev: return named opaque type instead of void pointer

[Neil Horman]

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