Re: [EXTERNAL] Re: [PATCH] [RFC] cryptodev: replace LIST_END enumerators with APIs

[Dodji Seketeli <dodji@redhat.com>]

Akhil Goyal <gakhil@marvell.com> writes:

>> >>> Now added inline APIs for getting the list end which need to be updated
>> >>> for each new entry to the enum. This shall help in avoiding ABI break
>> >>> for adding new algo.
>> >>>
>> >>
>> >> Hi Akhil,
>> >>
>> >> *I think* this hides the problem instead of fixing it, and this may be
>> >> partially because of the tooling (libabigail) limitation.
>> >>
>> >> This patch prevents the libabigail warning, true, but it doesn't improve
>> >> anything from the application's perspective.
>> >> Before or after this patch, application knows a fixed value as END value.
>> >>
>> >> Not all changes in the END (MAX) enum values cause ABI break, but tool
>> >> warns on all, that is why I think this may be tooling limitation [1].
>> >> (Just to stress, I am NOT talking about regular enum values change, I am
>> >> talking about only END (MAX) value changes caused by appending new enum
>> >> items.)
>> >>
>> >> As far as I can see (please Dodji, David correct me if I am wrong) ABI
>> >> break only happens if application and library exchange enum values in
>> >> the API (directly or within a struct).
>> >
>> > - There is also the following issue:
>> > A library publicly exports an array sized against a END (MAX) enum in the API.
>> > https://developers.redhat.com/blog/2019/05/06/how-c-array-sizes-become-part-of-the-binary-interface-of-a-library
>> >
>> 
>> I see. And Dodji explained this requires source code to detect.
>> 
>> I don't remember seeing a public array whose size is defined by an enum,
>> are you aware of any instance of this usage?
>
> https://patches.dpdk.org/project/dpdk/patch/20241009071151.1106-1-gmuthukrishn@marvell.com/
> This was merged yesterday.

I guess the problematic piece of the code is this:

    diff --git a/lib/cryptodev/rte_cryptodev.h
    b/lib/cryptodev/rte_cryptodev.h
    index bec947f6d5..aa6ef3a94d 100644
    --- a/lib/cryptodev/rte_cryptodev.h
    +++ b/lib/cryptodev/rte_cryptodev.h
    @@ -185,6 +185,9 @@  struct rte_cryptodev_asymmetric_xform_capability {
               * Value 0 means unavailable, and application should pass the
               required
                     * random value. Otherwise, PMD would internally compute
                     the random number.
                          */
    +
    +               uint32_t op_capa[RTE_CRYPTO_ASYM_OP_LIST_END];
    +                        /**< Operation specific capabilities. */
                             };


Is it possible for the struct rte_cryptodev_asymmetric_xform_capability
to be made an opaque struct which definition would be present only in a
.c file of the library?

Its data members would then be retrieved by getter functions that take a
pointer to that struct in parameter.

That way, the uint32_t op_capa[RTE_CRYPTO_ASYM_OP_LIST_END] data member
would be "private" to the .c file and thus would not be part of the
ABI.  Any change to the RTE_CRYPTO_ASYM_OP enum would then become
harmless to that struct.

I hope this helps.

-- 
		Dodji