Re: [dpdk-dev] [PATCH v4 2/4] mempool: add non-IO flag

[Olivier Matz <olivier.matz@6wind.com>]

On Fri, Oct 15, 2021 at 01:27:59PM +0000, Dmitry Kozlyuk wrote:
> > [...]
> > > +static int
> > > +test_mempool_flag_non_io_set_when_no_iova_contig_set(void)
> > > +{
> > > +     struct rte_mempool *mp;
> > > +     int ret;
> > > +
> > > +     mp = rte_mempool_create_empty("empty", MEMPOOL_SIZE,
> > > +                                   MEMPOOL_ELT_SIZE, 0, 0,
> > > +                                   SOCKET_ID_ANY,
> > MEMPOOL_F_NO_IOVA_CONTIG);
> > > +     RTE_TEST_ASSERT_NOT_NULL(mp, "Cannot create mempool: %s",
> > > +                              rte_strerror(rte_errno));
> > > +     rte_mempool_set_ops_byname(mp, rte_mbuf_best_mempool_ops(), NULL);
> > > +     ret = rte_mempool_populate_default(mp);
> > > +     RTE_TEST_ASSERT(ret > 0, "Failed to populate mempool: %s",
> > > +                     rte_strerror(rte_errno));
> > > +     RTE_TEST_ASSERT(mp->flags & MEMPOOL_F_NON_IO,
> > > +                     "NON_IO flag is not set when NO_IOVA_CONTIG is
> > set");
> > > +     rte_mempool_free(mp);
> > > +     return 0;
> > > +}
> > 
> > One comment that also applies to the previous patch. Using
> > RTE_TEST_ASSERT_*() is convenient to test a condition, display an error
> > message and return on error in one operation. But here it can cause a
> > leak on test failure.
> > 
> > I don't know what is the best approach to solve the issue. Having
> > equivalent test macros that do "goto fail" instead of "return -1" would
> > help here. I mean something like:
> >   RTE_TEST_ASSERT_GOTO_*(cond, label, fmt, ...)
> > 
> > What do you think?
> 
> This can work with existing macros:
> 
> 	#define TEST_TRACE_FAILURE(...) goto fail
> 
> Because of "trace" in the name it looks a bit like a hijacking.
> Probably the macro should be named TEST_HANDLE_FAILURE
> to suggest broader usages than just tracing,
> but for now it looks the most neat way.

That would work for me.
What about introducing another macro for this usage, that would
be "return -1" by default and that can be overridden?