[dpdk-dev,v4] ethdev: return named opaque type instead of void pointer
Checks
Commit Message
"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(-)
Comments
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.
Neil
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
>
> Neil
>
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
> >
>
>
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@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.
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
>>>
>>
>>
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@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.
>
> 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
> >>>
> >>
> >>
>
>
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@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.
>>
>> 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
>>>>>
>>>>
>>>>
>>
>>
@@ -142,13 +142,6 @@ Deprecation Notices
successful. This modification will only impact the PMDs, not the
applications.
-* ethdev: functions add rx/tx callback will return named opaque type
- ``rte_eth_add_rx_callback()``, ``rte_eth_add_first_rx_callback()`` and
- ``rte_eth_add_tx_callback()`` functions currently return callback object as
- ``void \*`` but APIs to delete callbacks get ``struct rte_eth_rxtx_callback \*``
- as parameter. For consistency functions adding callback will return
- ``struct rte_eth_rxtx_callback \*`` instead of ``void \*``.
-
* i40e: The default flexible payload configuration which extracts the first 16
bytes of the payload for RSS will be deprecated starting from 18.02. If
required the previous behavior can be configured using existing flow
@@ -3501,7 +3501,7 @@ rte_eth_dev_filter_ctrl(uint16_t port_id, enum rte_filter_type filter_type,
filter_op, arg));
}
-void *
+struct rte_eth_rxtx_callback *
rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_rx_callback_fn fn, void *user_param)
{
@@ -3543,7 +3543,7 @@ rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
return cb;
}
-void *
+struct rte_eth_rxtx_callback *
rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_rx_callback_fn fn, void *user_param)
{
@@ -3578,7 +3578,7 @@ rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
return cb;
}
-void *
+struct rte_eth_rxtx_callback *
rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
rte_tx_callback_fn fn, void *user_param)
{
@@ -3004,6 +3004,8 @@ int rte_eth_dev_filter_ctrl(uint16_t port_id, enum rte_filter_type filter_type,
int rte_eth_dev_get_dcb_info(uint16_t port_id,
struct rte_eth_dcb_info *dcb_info);
+struct rte_eth_rxtx_callback;
+
/**
* Add a callback to be called on packet RX on a given port and queue.
*
@@ -3028,7 +3030,8 @@ int rte_eth_dev_get_dcb_info(uint16_t port_id,
* NULL on error.
* On success, a pointer value which can later be used to remove the callback.
*/
-void *rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
+struct rte_eth_rxtx_callback *
+rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_rx_callback_fn fn, void *user_param);
/**
@@ -3056,7 +3059,8 @@ void *rte_eth_add_rx_callback(uint16_t port_id, uint16_t queue_id,
* NULL on error.
* On success, a pointer value which can later be used to remove the callback.
*/
-void *rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
+struct rte_eth_rxtx_callback *
+rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
rte_rx_callback_fn fn, void *user_param);
/**
@@ -3083,11 +3087,10 @@ void *rte_eth_add_first_rx_callback(uint16_t port_id, uint16_t queue_id,
* NULL on error.
* On success, a pointer value which can later be used to remove the callback.
*/
-void *rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
+struct rte_eth_rxtx_callback *
+rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
rte_tx_callback_fn fn, void *user_param);
-struct rte_eth_rxtx_callback;
-
/**
* Remove an RX packet callback from a given port and queue.
*