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

[Ferruh Yigit]

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 <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).

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
>>>>>
>>>>
>>>>
>>
>>