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

[Anders Roxell]

On Sat, 30 Oct 2021 at 00:08, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote:
>
> On Fri, Oct 29, 2021 at 11:19 AM Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On 10/29/21 5:43 AM, Anders Roxell wrote:
> > > 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 <anders.roxell@xxxxxxxxxx>
> >
> > 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.

>
> 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.


Cheers,
Anders