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

["Mattias Rönnblom" <hofors@lysator.liu.se>]

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.

>   	lmain.bit = rte_rand_max(size); \
>   	do { \
>   		lworker.bit = rte_rand_max(size); \
> @@ -218,6 +219,7 @@ test_bit_atomic_parallel_test_and_modify ## 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"); \
>   	int rc = rte_eal_remote_launch(run_parallel_test_and_modify ## size, &lworker, \
>   		worker_lcore_id); \
>   	TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
> @@ -267,6 +269,7 @@ test_bit_atomic_parallel_flip ## 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"); \
>   	int rc = rte_eal_remote_launch(run_parallel_flip ## size, &lworker, worker_lcore_id); \
>   	TEST_ASSERT(rc == 0, "Worker thread launch failed"); \
>   	run_parallel_flip ## size(&lmain); \