Re: [Devel] [PATCH hci-8.1 6/6] selftests/futex: fail futex_waitv when the waiter failed

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/futex: futex_waitv double-reports subtest when waiterfn() fails
--

commit d7a86a7b77fbc111f003b6a1f5173be520b5ac79
Author: Eva Kurchatova <eva.kurchatova@virtuozzo.com>

selftests/futex: fail futex_waitv when the waiter failed

This patch joins the waiter thread after each futex_wake() in the private
and shared subtests of futex_waitv. It also sets the exit status to
RET_FAIL if ksft_get_fail_cnt() is non-zero at the end of main().

Link: https://virtuozzo.atlassian.net/browse/VSTOR-134205

> diff --git a/tools/testing/selftests/futex/functional/futex_waitv.c b/tools/testing/selftests/futex/functional/futex_waitv.c
> --- a/tools/testing/selftests/futex/functional/futex_waitv.c
> +++ b/tools/testing/selftests/futex/functional/futex_waitv.c

[ ... ]

> @@ -100,6 +100,7 @@ int main(int argc, char *argv[])
>  	usleep(WAKE_WAIT_US);
>  
>  	res = futex_wake(u64_to_ptr(waitv[NR_FUTEXES - 1].uaddr), 1, FUTEX_PRIVATE_FLAG);
> +	pthread_join(waiter, NULL);
>  	if (res != 1) {
>  		ksft_test_result_fail("futex_wake private returned: %d %s\n",
>  				      res ? errno : res,
>  				      res ? strerror(errno) : "");
>  		ret = RET_FAIL;
>  	} else {
>  		ksft_test_result_pass("futex_waitv private\n");
>  	}

[Severity: Medium]
Can this report the same subtest twice?

If waiterfn() fails, it calls ksft_test_result_fail() itself:

waiterfn()
    res = futex_waitv(waitv, NR_FUTEXES, 0, &to, CLOCK_MONOTONIC);
    if (res < 0) {
        ksft_test_result_fail("futex_waitv returned: %d %s\n", ...);

The pthread_join() here discards the waiter's return value. If
futex_wake() then returns 1, main() still reaches
ksft_test_result_pass("futex_waitv private\n").

That would print both a "not ok" and an "ok" line for the same subtest.
The total number of results would then exceed ksft_set_plan(7).

The same applies to the shared case, which ends in
ksft_test_result_pass("futex_waitv shared\n").

Would it be cleaner for waiterfn() to return its status, so that main()
collects it through pthread_join() and reports a single result per subtest?

[ ... ]

> @@ -232,6 +234,10 @@ int main(int argc, char *argv[])
>  		ksft_test_result_pass("futex_waitv invalid clockid\n");
>  	}
>  
> +	/* The waiter records its own failures */
> +	if (ksft_get_fail_cnt())
> +		ret = RET_FAIL;
> +

[Severity: Medium]
Does this check only fix the exit code?

ksft_get_fail_cnt() is a global counter. It cannot tell a waiter failure
from a failure in main(). It also cannot attribute the failure to a
particular subtest.

Returning the waiter's status through pthread_join() would make this check
unnecessary.

>  	ksft_print_cnts();
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261026124139.606106-1-eva.kurchatova@virtuozzo.com?part=6

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel