On Wed, Apr 24, 2024 at 10:00:12AM -0600, Shuah Khan wrote:
On 4/24/24 09:05, Nathan Chancellor wrote:
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.
I am not asking each file to be a separate patch.
What granularity would you like? One per folder in tools/testing/selftest (i.e., clone3, ipc, membarrier, etc)?
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.
selftest patches go through various repos. With this patch touching several tests, there will be conflicts with multiple trees.
If this patch can't be split due to dependency on kselftest.h, I will pull it in, but I do need you to include all the maintainers.
No, it can be split as long as the kselftest.h change is last. I just did not see the value of that at the time but I am not out to make life harder for maintainers so I will split it as you see fit.
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.
The reason for cc'ing the maintainers is to keep them in the loop about this change that could result in merge conflicts between kselftest tree and theirs.
Besides I would rather not have developers make calls on who should or shouldn't care about a change. :)
Sure, that makes sense.
Cheers, Nathan