On Sun, Oct 13, 2024 at 8:53 AM Mattias Rönnblom <hof...@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.march...@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
