> > On 1/30/2020 8:18 PM, Thomas Monjalon wrote: > > 30/01/2020 17:09, Ferruh Yigit: > >> On 1/29/2020 8:13 PM, Akhil Goyal wrote: > >>> > >>> > >>>> > >>>> On Wed, Jan 29, 2020 at 7:10 PM Anoob Joseph <ano...@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. > > > > If library give higher value than expected by the application, > > if the application uses this value as array index, > > there can be an access out of bounds. > > First this concern is not an ABI break concern, but application should ignore > any value bigger than the MAX value it knows. > Otherwise this would mean we can't add any new enum or define to the project, > which is wrong I believe. > > > > > > >>>> [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. > > > > For this one, I don't know what is the breakage.
Reading through this report, I am also don't see why it is considered as ABI breakage. Yes, size of rte_crypto_aead_algorithm_strings[] has changed, but this array is not public one. Also I don't see any place where we use RTE_CRYPTO_AEAD_LIST_END to define array size in our public API. At first glance it looks like false positive to me. Do I miss something obvious here? Konstantin > > > > > >>> +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? > > > > > > > > > >
