Re: [dpdk-dev] [PATCH 05/15] security: switch metadata to dynamic mbuf field

[Thomas Monjalon <thomas@monjalon.net>]

26/10/2020 17:49, Thomas Monjalon:
> 26/10/2020 16:06, Andrew Rybchenko:
> > On 10/26/20 8:20 AM, Thomas Monjalon wrote:
> > [snip]
> > > -			sa = pkt->userdata;
> > > +			sa = RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset,
> > > +					struct ipsec_sa *);
> > 
> > I think it should be de-reference above, i.e.
> > sa = (struct ipsec_sa *)*RTE_MBUF_DYNFIELD(pkt,
> > security_dynfield_offset, RTE_SECURITY_DYNFIELD_TYPE *);
> > or just
> > sa = *RTE_MBUF_DYNFIELD(pkt, security_dynfield_offset, struct ipsec_sa **);
> 
> You're right something is wrong.
> I should add a dereference.
> 
> > and why not rte_security_dynfield()?
> 
> Because I need good reviews from you and David :)

The initial idea was to have a field lookup in the app,
and to not rely on what drivers have registered.
After more thoughts, the lookup can be replaced with a check
rte_security_dynfield_is_registered(),
so rte_security_dynfield() can be used.

> Will fix one and the other occurences.
> I've prepared a v2 using a macro, but I think I will follow
> your suggestion of having static inline functions.
> 
> > It all looks very fragile. May be at least add
> >    RTE_BUILD_BUG_ON(sizeof(RTE_SECURITY_DYNFIELD_TYPE) ==
> >                     sizeof(ipsec_sa *));
> > and similar checks when an application or a library does
> > lookup for a dynamic field.
> 
> You mean adding this after the lookup in the app?

I will remove the lookup, it is less fragile.