Re: [dpdk-dev] [PATCH v2 4/4] add ABI checks

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/29/2020 8:13 PM, Akhil Goyal wrote:
> 
> 
>>
>> On Wed, Jan 29, 2020 at 7:10 PM Anoob Joseph <anoobj@marvell.com> wrote:
>>> The asymmetric crypto library is experimental. Changes to experimental code
>> paths is allowed, right?
>>
>> The asymmetric crypto enum is referenced by a function part of the stable ABI.
>> It is possible to waive this enum, if we are sure no use out of the
>> experimental asym crypto APIs is possible.
>>
>> The rest of the changes touch stable symbols.
>>
>> Adding the abidiff report:
>>
>>   [C]'function void rte_cryptodev_info_get(uint8_t,
>> rte_cryptodev_info*)' at rte_cryptodev.c:1115:1 has some indirect
>> sub-type changes:
>>     parameter 2 of type 'rte_cryptodev_info*' has sub-type changes:
>>       in pointed to type 'struct rte_cryptodev_info' at rte_cryptodev.h:468:1:
>>         type size hasn't changed
>>         1 data member change:
>>          type of 'const rte_cryptodev_capabilities*
>> rte_cryptodev_info::capabilities' changed:
>>            in pointed to type 'const rte_cryptodev_capabilities':
>>              in unqualified underlying type 'struct
>> rte_cryptodev_capabilities' at rte_cryptodev.h:176:1:
>>                type size hasn't changed
>>                1 data member change:
>>                 type of '__anonymous_union__ ' changed:
>>                   type size hasn't changed
>>                   1 data member change:
>>                    type of 'rte_cryptodev_asymmetric_capability
>> __anonymous_union__::asym' changed:
>>                      type size hasn't changed
>>                      1 data member change:
>>                       type of
>> 'rte_cryptodev_asymmetric_xform_capability
>> rte_cryptodev_asymmetric_capability::xform_capa' changed:
>>                         type size hasn't changed
>>                         1 data member change:
>>                          type of 'rte_crypto_asym_xform_type
>> rte_cryptodev_asymmetric_xform_capability::xform_type' changed:
>>                            type size hasn't changed
>>                            2 enumerator insertions:
>>
>> 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_ECDSA' value '7'
>>
>> 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_ECPM' value '8'
>>                            1 enumerator change:
>>
>> 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END'
>> from
>> value '7' to '9' at rte_crypto_asym.h:60:1
>>
> 
> I believe these enums will be used only in case of ASYM case which is experimental.

Independent from being experiment and not, this shouldn't be a problem, I think
this is a false positive.

The ABI break can happen when a struct has been shared between the application
and the library (DPDK) and the layout of that memory know differently by
application and the library.

Here in all cases, there is no layout/size change.

As to the value changes of the enums, since application compiled with old DPDK,
it will know only up to '6', 7 and more means invalid to the application. So it
won't send these values also it should ignore these values from library. Only
consequence is old application won't able to use new features those new enums
provide but that is expected/normal.

> 
>>
>>   [C]'function int
>> rte_cryptodev_get_aead_algo_enum(rte_crypto_aead_algorithm*, const
>> char*)' at rte_cryptodev.c:239:1 has some indirect sub-type changes:
>>     parameter 1 of type 'rte_crypto_aead_algorithm*' has sub-type changes:
>>       in pointed to type 'enum rte_crypto_aead_algorithm' at
>> rte_crypto_sym.h:346:1:
>>         type size hasn't changed
>>         1 enumerator insertion:
>>           'rte_crypto_aead_algorithm::RTE_CRYPTO_AEAD_CHACHA20_POLY1305'
>> value '3'
>>         1 enumerator change:
>>           'rte_crypto_aead_algorithm::RTE_CRYPTO_AEAD_LIST_END' from
>> value '3' to '4' at rte_crypto_sym.h:346:1

Same as above, no layout change.

>>
>>
>>   [C]'const char* rte_crypto_aead_algorithm_strings[1]' was changed at
>> rte_crypto_sym.h:358:1:
>>     size of symbol (in bytes) changed from 24 to 32
>>

The shared memory size changes, but this is global variable in the library, and
the values application can request 'RTE_CRYPTO_AEAD_AES_CCM' &
'RTE_CRYPTO_AEAD_AES_GCM' is already there, so there is no backward
compatibility issue here.

>>
> +Fiona and Arek 
> 
> We may need to revert the chacha-poly patches.
> 

I don't see any ABI break in this case, can someone explain if I am missing
anything here?