diff mbox

[dpdk-dev,7/7] ethdev: use opaque user callback object

Message ID 20171201022957.64329-7-ferruh.yigit@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Ferruh Yigit Dec. 1, 2017, 2:29 a.m. UTC 
  "struct rte_eth_rxtx_callback" is defined as internal data structure but
used in public APIs.

Checking the API documentation shows that intention was using this
object as opaque object. Data structure only used in delete APIs which
doesn't require to know the internals of the data structure.

Converting callback parameter in API to void pointer should not require
any modification in user application because this data structure was
already marked as internal and only should be used as pointer in
application.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 10 ++++++----
 lib/librte_ether/rte_ethdev.h | 10 ++++------
 2 files changed, 10 insertions(+), 10 deletions(-)

Comments

Bruce Richardson Dec. 1, 2017, 10:33 a.m. UTC | #1 
On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote:
> "struct rte_eth_rxtx_callback" is defined as internal data structure but
> used in public APIs.
> 
> Checking the API documentation shows that intention was using this
> object as opaque object. Data structure only used in delete APIs which
> doesn't require to know the internals of the data structure.
> 
> Converting callback parameter in API to void pointer should not require
> any modification in user application because this data structure was
> already marked as internal and only should be used as pointer in
> application.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---

I disagree on this patch. The structure itself is not exposed, only the
name, since it is only passed around as a pointer, so there is no need
to change the parameters to void pointer. It's a named opaque type.
Ananyev, Konstantin Dec. 1, 2017, 11:22 a.m. UTC | #2 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Friday, December 1, 2017 10:33 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; vladz@cloudius-systems.com
> Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object
> 
> On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote:
> > "struct rte_eth_rxtx_callback" is defined as internal data structure but
> > used in public APIs.
> >
> > Checking the API documentation shows that intention was using this
> > object as opaque object. Data structure only used in delete APIs which
> > doesn't require to know the internals of the data structure.
> >
> > Converting callback parameter in API to void pointer should not require
> > any modification in user application because this data structure was
> > already marked as internal and only should be used as pointer in
> > application.
> >
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> 
> I disagree on this patch. The structure itself is not exposed, only the
> name, since it is only passed around as a pointer, so there is no need
> to change the parameters to void pointer. It's a named opaque type.

Personally I think it would be better to do visa-versa: 
make rte_eth_add_(rx|tx)_callback() to return struct rte_eth_rxtx_callback *
instead of void *.
Konstantin
Bruce Richardson Dec. 1, 2017, 1:17 p.m. UTC | #3 
On Fri, Dec 01, 2017 at 11:22:12AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Friday, December 1, 2017 10:33 AM
> > To: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; vladz@cloudius-systems.com
> > Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object
> > 
> > On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote:
> > > "struct rte_eth_rxtx_callback" is defined as internal data structure but
> > > used in public APIs.
> > >
> > > Checking the API documentation shows that intention was using this
> > > object as opaque object. Data structure only used in delete APIs which
> > > doesn't require to know the internals of the data structure.
> > >
> > > Converting callback parameter in API to void pointer should not require
> > > any modification in user application because this data structure was
> > > already marked as internal and only should be used as pointer in
> > > application.
> > >
> > > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > ---
> > 
> > I disagree on this patch. The structure itself is not exposed, only the
> > name, since it is only passed around as a pointer, so there is no need
> > to change the parameters to void pointer. It's a named opaque type.
> 
> Personally I think it would be better to do visa-versa: 
> make rte_eth_add_(rx|tx)_callback() to return struct rte_eth_rxtx_callback *
> instead of void *.
> Konstantin
> 
I didn't realise that it did, so definite +1 to that suggestion.
Ferruh Yigit Dec. 1, 2017, 11:51 p.m. UTC | #4 
On 12/1/2017 5:17 AM, Bruce Richardson wrote:
> On Fri, Dec 01, 2017 at 11:22:12AM +0000, Ananyev, Konstantin wrote:
>>
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>> Sent: Friday, December 1, 2017 10:33 AM
>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>>> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; vladz@cloudius-systems.com
>>> Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object
>>>
>>> On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote:
>>>> "struct rte_eth_rxtx_callback" is defined as internal data structure but
>>>> used in public APIs.
>>>>
>>>> Checking the API documentation shows that intention was using this
>>>> object as opaque object. Data structure only used in delete APIs which
>>>> doesn't require to know the internals of the data structure.
>>>>
>>>> Converting callback parameter in API to void pointer should not require
>>>> any modification in user application because this data structure was
>>>> already marked as internal and only should be used as pointer in
>>>> application.
>>>>
>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>> ---
>>>
>>> I disagree on this patch. The structure itself is not exposed, only the
>>> name, since it is only passed around as a pointer, so there is no need
>>> to change the parameters to void pointer. It's a named opaque type.
>>
>> Personally I think it would be better to do visa-versa: 
>> make rte_eth_add_(rx|tx)_callback() to return struct rte_eth_rxtx_callback *
>> instead of void *.
>> Konstantin
>>
> I didn't realise that it did, so definite +1 to that suggestion.

No issue on having a named opaque type, but unfortunately struct is exposed
because of inline functions again.
It has been moved into rte_ethdev_core.h but accessible by applications.

And since intention is an opaque type, because of "void *" return types, I
thought it is better to hide type completely so that application can't access
details.
Bruce Richardson Dec. 4, 2017, 10:31 a.m. UTC | #5 
On Fri, Dec 01, 2017 at 03:51:52PM -0800, Ferruh Yigit wrote:
> On 12/1/2017 5:17 AM, Bruce Richardson wrote:
> > On Fri, Dec 01, 2017 at 11:22:12AM +0000, Ananyev, Konstantin wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> >>> Sent: Friday, December 1, 2017 10:33 AM
> >>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; vladz@cloudius-systems.com
> >>> Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object
> >>>
> >>> On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote:
> >>>> "struct rte_eth_rxtx_callback" is defined as internal data structure but
> >>>> used in public APIs.
> >>>>
> >>>> Checking the API documentation shows that intention was using this
> >>>> object as opaque object. Data structure only used in delete APIs which
> >>>> doesn't require to know the internals of the data structure.
> >>>>
> >>>> Converting callback parameter in API to void pointer should not require
> >>>> any modification in user application because this data structure was
> >>>> already marked as internal and only should be used as pointer in
> >>>> application.
> >>>>
> >>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>> ---
> >>>
> >>> I disagree on this patch. The structure itself is not exposed, only the
> >>> name, since it is only passed around as a pointer, so there is no need
> >>> to change the parameters to void pointer. It's a named opaque type.
> >>
> >> Personally I think it would be better to do visa-versa: 
> >> make rte_eth_add_(rx|tx)_callback() to return struct rte_eth_rxtx_callback *
> >> instead of void *.
> >> Konstantin
> >>
> > I didn't realise that it did, so definite +1 to that suggestion.
> 
> No issue on having a named opaque type, but unfortunately struct is exposed
> because of inline functions again.
> It has been moved into rte_ethdev_core.h but accessible by applications.
> 
> And since intention is an opaque type, because of "void *" return types, I
> thought it is better to hide type completely so that application can't access
> details.

I wouldn't be worried about applications being able to get into the
structure. The only compelling reason for me to make the type opaque
would be for ABI compatibility, and since that is not a factor here, I
don't see the point in changing it to a void *.

/Bruce
Ferruh Yigit Dec. 4, 2017, 5:49 p.m. UTC | #6 
On 12/4/2017 2:31 AM, Bruce Richardson wrote:
> On Fri, Dec 01, 2017 at 03:51:52PM -0800, Ferruh Yigit wrote:
>> On 12/1/2017 5:17 AM, Bruce Richardson wrote:
>>> On Fri, Dec 01, 2017 at 11:22:12AM +0000, Ananyev, Konstantin wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
>>>>> Sent: Friday, December 1, 2017 10:33 AM
>>>>> To: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org; vladz@cloudius-systems.com
>>>>> Subject: Re: [dpdk-dev] [PATCH 7/7] ethdev: use opaque user callback object
>>>>>
>>>>> On Fri, Dec 01, 2017 at 02:29:57AM +0000, Ferruh Yigit wrote:
>>>>>> "struct rte_eth_rxtx_callback" is defined as internal data structure but
>>>>>> used in public APIs.
>>>>>>
>>>>>> Checking the API documentation shows that intention was using this
>>>>>> object as opaque object. Data structure only used in delete APIs which
>>>>>> doesn't require to know the internals of the data structure.
>>>>>>
>>>>>> Converting callback parameter in API to void pointer should not require
>>>>>> any modification in user application because this data structure was
>>>>>> already marked as internal and only should be used as pointer in
>>>>>> application.
>>>>>>
>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>> ---
>>>>>
>>>>> I disagree on this patch. The structure itself is not exposed, only the
>>>>> name, since it is only passed around as a pointer, so there is no need
>>>>> to change the parameters to void pointer. It's a named opaque type.
>>>>
>>>> Personally I think it would be better to do visa-versa: 
>>>> make rte_eth_add_(rx|tx)_callback() to return struct rte_eth_rxtx_callback *
>>>> instead of void *.
>>>> Konstantin
>>>>
>>> I didn't realise that it did, so definite +1 to that suggestion.
>>
>> No issue on having a named opaque type, but unfortunately struct is exposed
>> because of inline functions again.
>> It has been moved into rte_ethdev_core.h but accessible by applications.
>>
>> And since intention is an opaque type, because of "void *" return types, I
>> thought it is better to hide type completely so that application can't access
>> details.
> 
> I wouldn't be worried about applications being able to get into the
> structure. The only compelling reason for me to make the type opaque
> would be for ABI compatibility, and since that is not a factor here, I
> don't see the point in changing it to a void *.

OK, I will update as suggested.

> 
> /Bruce
>
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 5558db7c4..fca4895a5 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3206,12 +3206,13 @@  rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
 }
 
 int
-rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb)
+rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id, void *_user_cb)
 {
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	return -ENOTSUP;
 #endif
+	struct rte_eth_rxtx_callback *user_cb = _user_cb;
+
 	/* Check input parameters. */
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 	if (user_cb == NULL ||
@@ -3240,12 +3241,13 @@  rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
 }
 
 int
-rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb)
+rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id, void *_user_cb)
 {
 #ifndef RTE_ETHDEV_RXTX_CALLBACKS
 	return -ENOTSUP;
 #endif
+	struct rte_eth_rxtx_callback *user_cb = _user_cb;
+
 	/* Check input parameters. */
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 	if (user_cb == NULL ||
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 12b85a263..b94abef5f 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1215,8 +1215,6 @@  typedef uint16_t (*rte_rx_callback_fn)(uint16_t port_id, uint16_t queue,
 typedef uint16_t (*rte_tx_callback_fn)(uint16_t port_id, uint16_t queue,
 	struct rte_mbuf *pkts[], uint16_t nb_pkts, void *user_param);
 
-struct rte_eth_rxtx_callback;
-
 /**
  * A set of values to describe the possible states of an eth device.
  */
@@ -2960,8 +2958,8 @@  void *rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
  *   - -EINVAL:  The port_id or the queue_id is out of range, or the callback
  *               is NULL or not found for the port/queue.
  */
-int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb);
+int
+rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id, void *user_cb);
 
 /**
  * Remove a TX packet callback from a given port and queue.
@@ -2993,8 +2991,8 @@  int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
  *   - -EINVAL:  The port_id or the queue_id is out of range, or the callback
  *               is NULL or not found for the port/queue.
  */
-int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
-		struct rte_eth_rxtx_callback *user_cb);
+int
+rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id, void *user_cb);
 
 /**
  * Retrieve information about given port's RX queue.