26/10/2020 16:06, Andrew Rybchenko: > On 10/26/20 8:20 AM, Thomas Monjalon wrote: > > --- a/drivers/net/ixgbe/ixgbe_ipsec.c > > +++ b/drivers/net/ixgbe/ixgbe_ipsec.c > > - (union ixgbe_crypto_tx_desc_md *)&m->udata64; > > + (union ixgbe_crypto_tx_desc_md *) > > + rte_security_dynfield(m); > > IMHO alignment looks a bit confusing here, may be add one more > TAB?
OK, no opinion > [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 :) 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? > In general since lookup should not happen on data path, > may be lookup should return size of the field which must > be checked by the caller for consistency. It does. I can add a check. > Is it really different types of value in one example > application? It looks like it should be different > dynamic fields. Otherwise, I don't understand how > to work with it. Yes, udata64 was a trash bin used to store different kind of data, even inside librte_security. > In fact may be above is out of scope of the patch series... Yes, I think moving to a dedicated field is a first step. Second step (not by me) will be to split in different fields.
