Re: [PATCH] selftests: kselftest.h: mark functions with 'noreturn'

On Wed, Nov 03, 2021 at 10:38:17AM +0100, Anders Roxell wrote:

On Tue, 2 Nov 2021 at 23:04, Anders Roxell [off-list ref] wrote:

quoted

On Sat, 30 Oct 2021 at 00:08, Nick Desaulniers [off-list ref] wrote:

quoted

On Fri, Oct 29, 2021 at 11:19 AM Shuah Khan [off-list ref] wrote:

quoted

On 10/29/21 5:43 AM, Anders Roxell wrote:

quoted

When building kselftests/capabilities the following warning shows up:

clang -O2 -g -std=gnu99 -Wall    test_execve.c -lcap-ng -lrt -ldl -o test_execve
test_execve.c:121:13: warning: variable 'have_outer_privilege' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
         } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_execve.c:136:9: note: uninitialized use occurs here
         return have_outer_privilege;
                ^~~~~~~~~~~~~~~~~~~~
test_execve.c:121:9: note: remove the 'if' if its condition is always true
         } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test_execve.c:94:27: note: initialize the variable 'have_outer_privilege' to silence this warning
         bool have_outer_privilege;
                                  ^
                                   = false

Rework so all the ksft_exit_*() functions have attribue
'__attribute__((noreturn))' so the compiler knows that there wont be
any return from the function. That said, without
'__attribute__((noreturn))' the compiler warns about the above issue
since it thinks that it will get back from the ksft_exit_skip()
function, which it wont.
Cleaning up the callers that rely on ksft_exit_*() return code, since
the functions ksft_exit_*() have never returned anything.

Signed-off-by: Anders Roxell <redacted>

Lot of changes to fix this warning. Is this necessary? I would
like to explore if there is an easier and localized change that
can fix the problem.

via `man 3 exit`:

The  exit() function causes normal process termination ...
...
RETURN VALUE
       The exit() function does not return.

so seeing `ksft_exit_pass`, `ksft_exit_fail`, `ksft_exit_fail_msg`,
`ksft_exit_xfail`, `ksft_exit_xpass`, and `ksft_exit_skip` all
unconditional call `exit` yet return an `int` looks wrong to me on
first glance. So on that point this patch and its resulting diffstat
LGTM.

I'll respin the patch with these changes only.

quoted

That said, there are many changes that explicitly call `ksft_exit`
with an expression; are those setting the correct exit code? Note that
ksft_exit_pass is calling exit with KSFT_PASS which is 0.  So some of
the negations don't look quite correct to me.  For example:

-       return !ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail();
+       ksft_exit(!ksft_get_fail_cnt());

so if ksft_get_fail_cnt() returns 0, then we were calling
ksft_exit_pass() which exited with 0. Now we'd be exiting with 1?

oh, right, thank you for your review.
I will drop all the 'ksft_exit()' changes, they should be fixed and go
in as separete patches.

tools/testing/selftests/vm/memfd_secret.c has the
'ksft_exit(!ksft_get_fail_cnt())'
statement and when I looked at it it when I did this patch it looked correct.
However, when I look at it now I get a bit confused how ksft_exit() can be used
with ksft_get_fail_cnt(). @Mike can you please clarify the
'ksft_exit(!ksft_get_fail_cnt())' instance in
tools/testing/selftests/vm/memfd_secret.c.

ksft_exit() does not take the error code but rather a condition:

/**
 * ksft_exit() - Exit selftest based on truth of condition
 *
 * @condition: if true, exit self test with success, otherwise fail.
 */
#define ksft_exit(condition) do {	\
	if (!!(condition))		\
		ksft_exit_pass();	\
	else				\
		ksft_exit_fail();	\
	} while (0)

So

	!ksft_get_fail_cnt() ? ksft_exit_pass() : ksft_exit_fail();

and

	ksft_exit(!ksft_get_fail_cnt())

are equivalent.

-- 
Sincerely yours,
Mike.