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.
