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

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

On 10/4/2024 8:04 AM, David Marchand wrote:
> On Fri, Oct 4, 2024 at 5:55 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>>
>> On 9/5/2024 11:14 AM, Akhil Goyal wrote:
>>> Replace *_LIST_END enumerators from asymmetric crypto
>>> lib to avoid ABI breakage for every new addition in
>>> enums with inline APIs.
>>>
>>> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
>>> ---
>>> This patch was discussed in ML long time back.
>>> https://patches.dpdk.org/project/dpdk/patch/20211008204516.3497060-1-gakhil@marvell.com/
>>> 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?

> I had made comments for an issue in the cryptodev library in the past:
> https://inbox.dpdk.org/dev/CAJFAV8xs5CVdE2xwRtaxk5vE_PiQMV5LY5tKStk3R1gOuRTsUw@mail.gmail.com/
> 
> 
> - Sorry to deviate from the _END enum discussion that tries to define
> a solution for all cases, but all I see in the cryptodev patch is a
> need for an enumerator... for an internal unit test.
> From the RFC patch, I would simply change the
> rte_crypto_asym_xform_type_list_end helper into a non inline symbol
> and mark it __rte_internal, or move this helper to a non public header
> used by the unit test.
> 
> Or add a (internal) rte_crypto_asym_xform_type_next_op() used through
> a a RTE_CRYPTO_FOREACH_ASYM_OP() macro.
> 
>