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

[David Marchand <david.marchand@redhat.com>]

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


-- 
David Marchand