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.yi...@intel.com> > >>>> Acked-by: Stephen Hemminger <step...@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 > >>> > >> > >> > >
