27/10/2020 11:05, Olivier Matz:
> On Mon, Oct 26, 2020 at 11:20:03PM +0100, Thomas Monjalon wrote:
> > The device-specific metadata was stored in the deprecated field udata64.
> > It is moved to a dynamic mbuf field in order to allow removal of udata64.
> >
> > Signed-off-by: Thomas Monjalon <tho...@monjalon.net>
> <...>
> > /* Save security session */
> > - pkt->userdata = sess_tbl[port_id];
> > + if (rte_security_dynfield_is_registered())
> > + *(struct rte_security_session **)
> > + rte_security_dynfield(pkt) =
> > + sess_tbl[port_id];
> >
>
> Maybe the last 2 lines can be on the same line (a bit more than 80,
> but less than 100 chars).
I prefer limiting to 80 if possible.
> This is not clear to me why you need to call
> rte_security_dynfield_is_registered() here, and not in other places.
I think other places are paths for rte_security only.
> Would it make sense instead to always register the dynfield at some
> place in rte_security, so that we are sure that as soon as we use
> rte_security, the dynfield is registered. A good place would be an init
> function, but I don't see one.
Indeed there is no such place in rte_security.
I keep thinking this is not a real library.
> > +bool
> > +rte_security_dynfield_is_registered(void)
> > +{
> > + return rte_security_dynfield_offset >= 0;
> > +}
> > +
>
> Wouldn't it be better to have it in a static inline function?
> The point is just to check that offset is >= 0, and it is used
> in data path.
Yes I'll make it inline
> > +/** Device-specific metadata field type */
> > +#define RTE_SECURITY_DYNFIELD_TYPE uint64_t
>
> What about using a typedef instead of a #define?
> It could be in lowercase: rte_security_dynfield_t
Yes
