Received-SPF: pass (google.com: domain of linux-kernel+bounces-157154-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 24 Apr 2024 08:05:13 -0700
From: Nathan Chancellor <nathan@kernel.org>
To: Shuah Khan <skhan@linuxfoundation.org>
Cc: shuah@kernel.org, tglx@linutronix.de, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] selftests: Make ksft_exit functions return void instead
 of int
Message-ID: <20240424150513.GA304587@dev-arch.thelio-3990X>
References: <20240417-ksft-exit-int-to-void-v1-1-eff48fdbab39@kernel.org>
 <bb5fd480-bd43-42c9-b326-2ee7addcda33@linuxfoundation.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <bb5fd480-bd43-42c9-b326-2ee7addcda33@linuxfoundation.org>

On Wed, Apr 24, 2024 at 07:44:31AM -0600, Shuah Khan wrote:
> On 4/17/24 09:37, Nathan Chancellor wrote:
> > Commit f7d5bcd35d42 ("selftests: kselftest: Mark functions that
> > unconditionally call exit() as __noreturn") marked functions that call
> > exit() as __noreturn but it did not change the return type of these
> > functions from 'void' to 'int' like it should have (since a noreturn
> > function by definition cannot return an integer because it does not
> > return...) because there are many tests that return the result of the
> > ksft_exit function, even though it has never been used due to calling
> > exit().
> > 
> > Prior to adding __noreturn, the compiler would not know that the functions
> > that call exit() will not return, so code like
> > 
> >    void ksft_exit_fail(void)
> >    {
> >      exit(1);
> >    }
> > 
> >    void ksft_exit_pass(void)
> >    {
> >      exit(0);
> >    }
> > 
> >    int main(void)
> >    {
> >      int ret;
> > 
> >      ret = foo();
> >      if (ret)
> >        ksft_exit_fail();
> >      ksft_exit_pass();
> >    }
> > 
> > would cause the compiler to complain that main() does not return an
> > integer, even though when ksft_exit_pass() is called, exit() will cause
> > the program to terminate. So ksft_exit_...() returns int to make the
> > compiler happy.
> > 
> >    int ksft_exit_fail(void)
> >    {
> >      exit(1);
> >    }
> > 
> >    int ksft_exit_pass(void)
> >    {
> >      exit(0);
> >    }
> > 
> >    int main(void)
> >    {
> >      int ret;
> > 
> >      ret = foo();
> >      if (ret)
> >        return ksft_exit_fail();
> >      return ksft_exit_pass();
> >    }
> > 
> > While this results in no warnings, it is weird semantically and it has
> > issues as noted in the aforementioned __noreturn change. Now that
> > __noreturn has been added to these functions, it is much cleaner to
> > change the functions to 'void' and eliminate the return statements, as
> > it has been made clear to the compiler that these functions terminate
> > the program. Drop the return before all instances of ksft_exit_...() in
> > a mechanical way. Only two manually changes were made to transform
> > 
> >    return !ret ? ksft_exit_pass() : ksft_exit_fail();
> > 
> > into the more idiomatic
> > 
> >    if (ret)
> >      ksft_exit_fail();
> >    ksft_exit_pass();
> > 
> > as well as a few style clean ups now that the code is shorter.
> > 
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> > ---
> >   tools/testing/selftests/clone3/clone3_clear_sighand.c        |  2 +-
> >   tools/testing/selftests/clone3/clone3_set_tid.c              |  4 +++-
> >   tools/testing/selftests/ipc/msgque.c                         | 11 +++++------
> >   tools/testing/selftests/kselftest.h                          | 12 ++++++------
> >   .../selftests/membarrier/membarrier_test_multi_thread.c      |  2 +-
> >   .../selftests/membarrier/membarrier_test_single_thread.c     |  2 +-
> >   tools/testing/selftests/mm/compaction_test.c                 |  6 +++---
> >   tools/testing/selftests/mm/cow.c                             |  2 +-
> >   tools/testing/selftests/mm/gup_longterm.c                    |  2 +-
> >   tools/testing/selftests/mm/gup_test.c                        |  4 ++--
> >   tools/testing/selftests/mm/ksm_functional_tests.c            |  2 +-
> >   tools/testing/selftests/mm/madv_populate.c                   |  2 +-
> >   tools/testing/selftests/mm/mkdirty.c                         |  2 +-
> >   tools/testing/selftests/mm/pagemap_ioctl.c                   |  4 ++--
> >   tools/testing/selftests/mm/soft-dirty.c                      |  2 +-
> >   tools/testing/selftests/pidfd/pidfd_fdinfo_test.c            |  2 +-
> >   tools/testing/selftests/pidfd/pidfd_open_test.c              |  4 +++-
> >   tools/testing/selftests/pidfd/pidfd_poll_test.c              |  2 +-
> >   tools/testing/selftests/pidfd/pidfd_test.c                   |  2 +-
> >   tools/testing/selftests/resctrl/resctrl_tests.c              |  6 +++---
> >   tools/testing/selftests/sync/sync_test.c                     |  3 +--
> >   tools/testing/selftests/timers/adjtick.c                     |  4 ++--
> >   tools/testing/selftests/timers/alarmtimer-suspend.c          |  4 ++--
> >   tools/testing/selftests/timers/change_skew.c                 |  4 ++--
> >   tools/testing/selftests/timers/freq-step.c                   |  4 ++--
> >   tools/testing/selftests/timers/leap-a-day.c                  | 10 +++++-----
> >   tools/testing/selftests/timers/leapcrash.c                   |  4 ++--
> >   tools/testing/selftests/timers/mqueue-lat.c                  |  4 ++--
> >   tools/testing/selftests/timers/posix_timers.c                | 12 ++++++------
> >   tools/testing/selftests/timers/raw_skew.c                    |  6 +++---
> >   tools/testing/selftests/timers/set-2038.c                    |  4 ++--
> >   tools/testing/selftests/timers/set-tai.c                     |  4 ++--
> >   tools/testing/selftests/timers/set-timer-lat.c               |  4 ++--
> >   tools/testing/selftests/timers/set-tz.c                      |  4 ++--
> >   tools/testing/selftests/timers/skew_consistency.c            |  4 ++--
> >   tools/testing/selftests/timers/threadtest.c                  |  2 +-
> >   tools/testing/selftests/timers/valid-adjtimex.c              |  6 +++---
> >   tools/testing/selftests/x86/lam.c                            |  2 +-
> >   38 files changed, 81 insertions(+), 79 deletions(-)
> > 
> 
> Please generate separate patches for each test so it is easy to apply
> them and also reduce merge conflicts.

Is applying 30+ patches easier than applying just one? It is not a
trivial amount of work for me to break this series up into individual
patches but I will do so if you really want me to. I based this on the
kselftest tree directly so that it would apply cleanly.

How does breaking apart the changes reduce merge conflicts? The diff is
going to be the same and semantic conflicts can still occur due to the
kselftest.h changes.

> You are missing maintainers for clone3, mm, pidfd tests. I can take these
> through kselftest tree, but I need the changes split.

Fair enough, I should have CC'd them, although given this is a change to
the kselftest API, I was not sure they would care too much.

Cheers,
Nathan