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

Ferruh Yigit ferruh.yigit at intel.com
Fri Mar 9 16:45:49 CET 2018


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

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.

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 *".

> 
> Neil
> 
>>>
>>> Neil
>>>
>>
>>



More information about the dev mailing list