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?
