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

[Akhil Goyal <gakhil@marvell.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/
> 
Yes this issue is being discussed multiple times.
One discussion is mentioned in patch description also.
There haven’t been a consensus yet.

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

That means we are not allowing applications to use it. Right?
So then why not we remove these LIST_END completely.
Test app can manage without it as well.
This was the original RFC to remove all list end.
https://patches.dpdk.org/project/dpdk/patch/20211008204516.3497060-1-gakhil@marvell.com/

As per our last discussion we need to find the LIST_END using a function and not via MACRO or enum.

This RFC tries to do that with an inline function which will give the list end based on the last enum value
Which is defined in that header file.
The only thing is we have an overhead of updating that inline function with a different value on every new addition which are normally not very frequent.

> 
> Or add a (internal) rte_crypto_asym_xform_type_next_op() used through
> a a RTE_CRYPTO_FOREACH_ASYM_OP() macro.
> 
> 
> --
> David Marchand