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

[Bruce Richardson]

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 at dpdk.org] On Behalf Of Bruce Richardson
> >>> Sent: Friday, December 1, 2017 10:33 AM
> >>> To: Yigit, Ferruh <ferruh.yigit at intel.com>
> >>> Cc: Thomas Monjalon <thomas at monjalon.net>; dev at dpdk.org; vladz at 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 at 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