Re: [dpdk-dev] [PATCH v3] mbuf: fix dynamic flags lookup from secondary process

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 26 Oct 2020 11:39:35 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:

> Hi Stephen,
> 
> On Fri, Oct 23, 2020 at 05:43:31PM -0700, Stephen Hemminger wrote:
> > The dynamic flag management is broken if rte_mbuf_dynflag_lookup()
> > is done in a secondary process because the local pointer to
> > the memzone is not ever initialized.
> > 
> > Fix it by using the same checks as dynfield_register().
> > I.e if shared memory zone has not been looked up already,
> > then discover it.
> > 
> > Fixes: 4958ca3a443a ("mbuf: support dynamic fields and flags")
> > Cc: olivier.matz@6wind.com
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > 
> > v3 - change title, fix one extra whitespace 
> > 
> >  lib/librte_mbuf/rte_mbuf_dyn.c | 20 ++++++++------------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf_dyn.c b/lib/librte_mbuf/rte_mbuf_dyn.c
> > index 538a43f6959f..554ec5a1ca4f 100644
> > --- a/lib/librte_mbuf/rte_mbuf_dyn.c
> > +++ b/lib/librte_mbuf/rte_mbuf_dyn.c
> > @@ -185,13 +185,11 @@ rte_mbuf_dynfield_lookup(const char *name, struct rte_mbuf_dynfield *params)
> >  {
> >  	struct mbuf_dynfield_elt *mbuf_dynfield;
> >  
> > -	if (shm == NULL) {
> > -		rte_errno = ENOENT;
> > -		return -1;
> > -	}
> > -
> >  	rte_mcfg_tailq_read_lock();
> > -	mbuf_dynfield = __mbuf_dynfield_lookup(name);
> > +	if (shm == NULL && init_shared_mem() < 0)
> > +		mbuf_dynfield = NULL;
> > +	else
> > +		mbuf_dynfield = __mbuf_dynfield_lookup(name);
> >  	rte_mcfg_tailq_read_unlock();
> >  
> >  	if (mbuf_dynfield == NULL) {
> >  		rte_errno = ENOENT;
> >  		return -1;  
> 
> There is still a small corner case here: on a primary process,
> init_shared_mem() can return -1 in case rte_memzone_reserve_aligned()
> returns a NULL memzone. In this situation, rte_errno is set by the
> memzone layer by overriden to ENOENT.
> 
> Maybe something like this is better, what do you think?

Sure, for what I was using rte_errno was not important. And since it was
previously broken lets get it fixed.