[dpdk-dev] [EXT] Re: [PATCH v10 1/4] devtools: add exception for reserved fields

[David Marchand]

On Thu, Apr 15, 2021 at 7:33 AM Akhil Goyal <gakhil at marvell.com> wrote:
>
> Hi David,
> > > Certain structures are added with reserved fields
> > > to address any future enhancements to retain ABI
> > > compatibility.
> > > However, ABI script will still report error as it
> > > is not aware of reserved fields. Hence, adding a
> > > generic exception for reserved fields.
> > >
> > > Signed-off-by: Akhil Goyal <gakhil at marvell.com>
> > > ---
> > >  devtools/libabigail.abignore | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > > index 6c0b38984..654755314 100644
> > > --- a/devtools/libabigail.abignore
> > > +++ b/devtools/libabigail.abignore
> > > @@ -19,4 +19,8 @@
> > >  ; Ignore fields inserted in cacheline boundary of rte_cryptodev
> > >  [suppress_type]
> > >          name = rte_cryptodev
> > > -        has_data_member_inserted_between = {offset_after(attached), end}
> > > \ No newline at end of file
> > > +        has_data_member_inserted_between = {offset_after(attached), end}
> > > +
> > > +; Ignore changes in reserved fields
> > > +[suppress_variable]
> > > +       name_regexp = reserved
> > Mm, this rule is a bit scary, as it matches anything with "reserved" in it.
>
> Why do you feel it is scary? Reserved is something which may change at any time
> Just like experimental. Hence creating a generic exception rule for it make sense
> And it is done intentionally in this patch.

The reserved regexp on the name of a variable / struct field is too lax.
Anything could be named with reserved in it.
If we have clear patterns, they must be preferred, like (untested)
name_regexp = ^reserved_(64|ptr)s$


Experimental is different.
This is a symbol version tag, which has a clear meaning and can't be
used for anything else.


>
> >
> > You need an exception anyway to insert the new fields (like in patch 2).
> > Can you test your series dropping this patch 1 ?
> It will not work, as there are 2 changes,
> 1. addition of ca_enqueue after attached. This is taken care by the exception set in patch 2
> 2. change in the reserved_ptr[4] -> reserved_ptr[3]. For this change we need exception for reserved.

In the eventdev struct, reserved fields are all in the range between
the attached field and the end of the struct.
I pushed your series without patch 1 to a branch of mine, and it
passes the check fine:
https://github.com/david-marchand/dpdk/commits/crypto_fwd_mode_v10
https://github.com/david-marchand/dpdk/runs/2350324578?check_suite_focus=true#step:15:8549


-- 
David Marchand