Re: [PATCH] test/bitops: check worker lcore availability

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

On Sun, Oct 13, 2024 at 8:53 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2024-10-11 17:25, David Marchand wrote:
> > Coverity is not able to understand that having 2 lcores means that
> > rte_get_next_lcore(-1, 0, 1) can't return RTE_MAX_LCORE.
> > Add an assert.
> >
> > Coverity issue: 445382, 445383, 445384, 445387, 445389, 445391
> > Fixes: 35326b61aecb ("bitops: add atomic bit operations in new API")
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > Note:
> > - a better fix would be to check lcore id validity in the EAL launch API,
> >    but it requires inspecting all functions and it could result in some
> >    API change, so sending this as a simple fix for now,
> >
> > ---
> >   app/test/test_bitops.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
> > index 4200073ae4..4ed54709fb 100644
> > --- a/app/test/test_bitops.c
> > +++ b/app/test/test_bitops.c
> > @@ -159,6 +159,7 @@ test_bit_atomic_parallel_assign ## size(void) \
> >               return TEST_SKIPPED; \
> >       } \
> >       worker_lcore_id = rte_get_next_lcore(-1, 1, 0); \
> > +     TEST_ASSERT(worker_lcore_id < RTE_MAX_LCORE, "Failed to find a worker lcore"); \
>
> How about:
>
> static unsigned int
> get_worker_lcore(void)
> {
>         unsigned int lcore_id;
>
>         lcore_id = rte_get_next_lcore(-1, 1, 0);
>
>         /* avoid Coverity false positives */
>         RTE_VERIFY(lcore_id < RTE_MAX_LCORE);
>
>         return lcore_id;
> }
>
> In the macros:
> worker_lcore_id = get_worker_lcore(-1, 1, 0);
>
> Makes the macros a tiny bit smaller/less redundant and gives an
> opportunity for a comment. Also, it's more appropriate to use RTE_VERIFY
> I would argue, since rte_get_next_lcore() is not the SUT.

I agree on the principle.
I will send a new one when possible.


-- 
David Marchand