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

[Ferruh Yigit]

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

OK, I will update as suggested.

> 
> /Bruce
>