Re: [dpdk-dev] [PATCH] mempool: enforce valid flags at creation

[David Marchand <david.marchand@redhat.com>]

On Tue, Oct 12, 2021 at 9:49 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
>
> On 10/12/21 10:28 AM, David Marchand wrote:
> > If we do not enforce valid flags are passed by an application, this
> > application might face issues in the future when we add more flags.
>
> Thanks. I'd even consider it as a bug and the fix to be
> backported.

I wondered too when writing the patch.
I'd like to hear from others.

>
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>
> A nit below, other than that:
>
> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>
> [snip]
>
> > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > index c5f859ae71..a2a78125f4 100644
> > --- a/lib/mempool/rte_mempool.c
> > +++ b/lib/mempool/rte_mempool.c
> > @@ -777,6 +777,13 @@ rte_mempool_cache_free(struct rte_mempool_cache *cache)
> >       rte_free(cache);
> >  }
> >
> > +#define MEMPOOL_KNOWN_FLAGS ( MEMPOOL_F_NO_SPREAD \
> > +     | MEMPOOL_F_NO_CACHE_ALIGN \
> > +     | MEMPOOL_F_SP_PUT \
> > +     | MEMPOOL_F_SC_GET \
> > +     | MEMPOOL_F_POOL_CREATED \
> > +     | MEMPOOL_F_NO_IOVA_CONTIG \
> > +     )
> >  /* create an empty mempool */
> >  struct rte_mempool *
> >  rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> > @@ -806,6 +813,12 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
> >                         RTE_CACHE_LINE_MASK) != 0);
> >  #endif
> >
> > +     /* enforce no unknown flag is passed by the application */
> > +     if ((flags & ~MEMPOOL_KNOWN_FLAGS) != 0) {
> > +             rte_errno = EINVAL;
> > +             return NULL;
> > +     }
> > +
>
> I think it is better to check arguments in parameters order.
> So, it is a bit better to move the check to happen after
> cache_size validation below.

Sounds reasonable, for v2.


-- 
David Marchand