[PATCH net-next 0/4] selftests: kselftest_harness: support using xfail

[PATCH net-next 0/4] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

Hi!

When running selftests for our subsystem in our CI we'd like all
tests to pass. Currently some tests use SKIP for cases they
expect to fail, because the kselftest_harness limits the return
codes to pass/fail/skip.

Clean up and support the use of the full range of ksft exit codes
under kselftest_harness.

To avoid conflicts and get the functionality into the networking
tree ASAP I'd like to put the patches on shared branch so that
both linux-kselftest and net-next can pull it in. Shuah, please
LMK if that'd work for you, and if so which -rc should I base
the branch on. Or is merging directly into net-next okay?

Jakub Kicinski (4):
  selftests: kselftest_harness: pass step via shared memory
  selftests: kselftest_harness: use KSFT_* exit codes
  selftests: kselftest_harness: support using xfail
  selftests: ip_local_port_range: use XFAIL instead of SKIP

 tools/testing/selftests/kselftest_harness.h   | 67 ++++++++++++++-----
 .../selftests/net/ip_local_port_range.c       |  2 +-
 2 files changed, 52 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH net-next 1/4] selftests: kselftest_harness: pass step via shared memory

[Jakub Kicinski]

Commit 0ef67a888375 ("selftests/harness: Report skip reason")
added shared memory to communicate between harness and test.
Use that instead of exit codes to send the failing step back
to the harness. The exit codes are limited and because of
the step passing we can't use the full range of KSFT_* exit
codes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index e05ac8261046..98bdedf9a53a 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -101,8 +101,8 @@
  * ASSERT_* number for which the test failed.  This behavior can be enabled by
  * writing `_metadata->no_print = true;` before the check sequence that is
  * unable to print.  When an error occur, instead of printing an error message
- * and calling `abort(3)`, the test process call `_exit(2)` with the assert
- * number as argument, which is then printed by the parent process.
+ * and calling `abort(3)`, the test process call `_exit(2)` and pass the error
+ * to be printed to the parent process via shared memory.
  */
 #define TH_LOG(fmt, ...) do { \
 	if (TH_LOG_ENABLED) \
@@ -695,9 +695,8 @@
 			__bail(_assert, _metadata))
 
 #define __INC_STEP(_metadata) \
-	/* Keep "step" below 255 (which is used for "SKIP" reporting). */	\
-	if (_metadata->passed && _metadata->step < 253) \
-		_metadata->step++;
+	if (_metadata->passed) \
+		_metadata->results->step++;
 
 #define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
 
@@ -784,6 +783,7 @@
 
 struct __test_results {
 	char reason[1024];	/* Reason for test result */
+	unsigned int step;	/* Test step reached without failure */
 };
 
 struct __test_metadata;
@@ -837,7 +837,6 @@ struct __test_metadata {
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
 	bool timed_out;	/* did this test timeout instead of exiting? */
-	__u8 step;
 	bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
 	bool aborted;	/* stopped test due to failed ASSERT */
 	bool setup_completed; /* did setup finish? */
@@ -875,7 +874,7 @@ static inline void __test_check_assert(struct __test_metadata *t)
 {
 	if (t->aborted) {
 		if (t->no_print)
-			_exit(t->step);
+			_exit(1);
 		abort();
 	}
 }
@@ -960,7 +959,7 @@ void __wait_for_test(struct __test_metadata *t)
 				fprintf(TH_LOG_STREAM,
 					"# %s: Test failed at step #%d\n",
 					t->name,
-					WEXITSTATUS(status));
+					t->results->step);
 			}
 		}
 	} else if (WIFSIGNALED(status)) {
@@ -1114,9 +1113,9 @@ void __run_test(struct __fixture_metadata *f,
 	t->passed = 1;
 	t->skip = 0;
 	t->trigger = 0;
-	t->step = 1;
 	t->no_print = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
+	t->results->step = 1;
 
 	ksft_print_msg(" RUN           %s%s%s.%s ...\n",
 	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
@@ -1137,8 +1136,8 @@ void __run_test(struct __fixture_metadata *f,
 		/* Pass is exit 0 */
 		if (t->passed)
 			_exit(0);
-		/* Something else happened, report the step. */
-		_exit(t->step);
+		/* Something else happened. */
+		_exit(1);
 	} else {
 		__wait_for_test(t);
 	}
-- 
2.43.0

Re: [PATCH net-next 1/4] selftests: kselftest_harness: pass step via shared memory

[Kees Cook]

On Tue, Feb 13, 2024 at 07:44:13AM -0800, Jakub Kicinski wrote:
> Commit 0ef67a888375 ("selftests/harness: Report skip reason")
> added shared memory to communicate between harness and test.
> Use that instead of exit codes to send the failing step back
> to the harness. The exit codes are limited and because of
> the step passing we can't use the full range of KSFT_* exit
> codes.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Ah, very nice! Good idea.

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH net-next 1/4] selftests: kselftest_harness: pass step via shared memory

[kernel test robot]

Hi Jakub,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Kicinski/selftests-kselftest_harness-pass-step-via-shared-memory/20240213-234644
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240213154416.422739-2-kuba%40kernel.org
patch subject: [PATCH net-next 1/4] selftests: kselftest_harness: pass step via shared memory
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240219/202402191034.76fuePpP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402191034.76fuePpP-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs_test.c:26:
   fs_test.c: In function 'layout1_no_restriction':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:403:1: note: in expansion of macro 'TEST_F_FORK'
     403 | TEST_F_FORK(layout1, no_restriction)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:403:1: note: in expansion of macro 'TEST_F_FORK'
     403 | TEST_F_FORK(layout1, no_restriction)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:403:1: note: in expansion of macro 'TEST_F_FORK'
     403 | TEST_F_FORK(layout1, no_restriction)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_inval':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:426:1: note: in expansion of macro 'TEST_F_FORK'
     426 | TEST_F_FORK(layout1, inval)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:426:1: note: in expansion of macro 'TEST_F_FORK'
     426 | TEST_F_FORK(layout1, inval)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:426:1: note: in expansion of macro 'TEST_F_FORK'
     426 | TEST_F_FORK(layout1, inval)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_file_and_dir_access_rights':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:548:1: note: in expansion of macro 'TEST_F_FORK'
     548 | TEST_F_FORK(layout1, file_and_dir_access_rights)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:548:1: note: in expansion of macro 'TEST_F_FORK'
     548 | TEST_F_FORK(layout1, file_and_dir_access_rights)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:548:1: note: in expansion of macro 'TEST_F_FORK'
     548 | TEST_F_FORK(layout1, file_and_dir_access_rights)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout0_ruleset_with_unknown_access':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:592:1: note: in expansion of macro 'TEST_F_FORK'
     592 | TEST_F_FORK(layout0, ruleset_with_unknown_access)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:592:1: note: in expansion of macro 'TEST_F_FORK'
     592 | TEST_F_FORK(layout0, ruleset_with_unknown_access)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:592:1: note: in expansion of macro 'TEST_F_FORK'
     592 | TEST_F_FORK(layout0, ruleset_with_unknown_access)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout0_rule_with_unknown_access':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:608:1: note: in expansion of macro 'TEST_F_FORK'
     608 | TEST_F_FORK(layout0, rule_with_unknown_access)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:608:1: note: in expansion of macro 'TEST_F_FORK'
     608 | TEST_F_FORK(layout0, rule_with_unknown_access)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:608:1: note: in expansion of macro 'TEST_F_FORK'
     608 | TEST_F_FORK(layout0, rule_with_unknown_access)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_rule_with_unhandled_access':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:635:1: note: in expansion of macro 'TEST_F_FORK'
     635 | TEST_F_FORK(layout1, rule_with_unhandled_access)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:635:1: note: in expansion of macro 'TEST_F_FORK'
     635 | TEST_F_FORK(layout1, rule_with_unhandled_access)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:635:1: note: in expansion of macro 'TEST_F_FORK'
     635 | TEST_F_FORK(layout1, rule_with_unhandled_access)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout0_proc_nsfs':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:741:1: note: in expansion of macro 'TEST_F_FORK'
     741 | TEST_F_FORK(layout0, proc_nsfs)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:741:1: note: in expansion of macro 'TEST_F_FORK'
     741 | TEST_F_FORK(layout0, proc_nsfs)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:741:1: note: in expansion of macro 'TEST_F_FORK'
     741 | TEST_F_FORK(layout0, proc_nsfs)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout0_unpriv':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:790:1: note: in expansion of macro 'TEST_F_FORK'
     790 | TEST_F_FORK(layout0, unpriv)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:790:1: note: in expansion of macro 'TEST_F_FORK'
     790 | TEST_F_FORK(layout0, unpriv)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:790:1: note: in expansion of macro 'TEST_F_FORK'
     790 | TEST_F_FORK(layout0, unpriv)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_effective_access':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:813:1: note: in expansion of macro 'TEST_F_FORK'
     813 | TEST_F_FORK(layout1, effective_access)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:813:1: note: in expansion of macro 'TEST_F_FORK'
     813 | TEST_F_FORK(layout1, effective_access)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:813:1: note: in expansion of macro 'TEST_F_FORK'
     813 | TEST_F_FORK(layout1, effective_access)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_unhandled_access':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:871:1: note: in expansion of macro 'TEST_F_FORK'
     871 | TEST_F_FORK(layout1, unhandled_access)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:871:1: note: in expansion of macro 'TEST_F_FORK'
     871 | TEST_F_FORK(layout1, unhandled_access)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:871:1: note: in expansion of macro 'TEST_F_FORK'
     871 | TEST_F_FORK(layout1, unhandled_access)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_ruleset_overlap':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:898:1: note: in expansion of macro 'TEST_F_FORK'
     898 | TEST_F_FORK(layout1, ruleset_overlap)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:898:1: note: in expansion of macro 'TEST_F_FORK'
     898 | TEST_F_FORK(layout1, ruleset_overlap)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:898:1: note: in expansion of macro 'TEST_F_FORK'
     898 | TEST_F_FORK(layout1, ruleset_overlap)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_layer_rule_unions':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:939:1: note: in expansion of macro 'TEST_F_FORK'
     939 | TEST_F_FORK(layout1, layer_rule_unions)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:939:1: note: in expansion of macro 'TEST_F_FORK'
     939 | TEST_F_FORK(layout1, layer_rule_unions)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:939:1: note: in expansion of macro 'TEST_F_FORK'
     939 | TEST_F_FORK(layout1, layer_rule_unions)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_non_overlapping_accesses':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1046:1: note: in expansion of macro 'TEST_F_FORK'
    1046 | TEST_F_FORK(layout1, non_overlapping_accesses)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1046:1: note: in expansion of macro 'TEST_F_FORK'
    1046 | TEST_F_FORK(layout1, non_overlapping_accesses)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1046:1: note: in expansion of macro 'TEST_F_FORK'
    1046 | TEST_F_FORK(layout1, non_overlapping_accesses)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_interleaved_masked_accesses':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1095:1: note: in expansion of macro 'TEST_F_FORK'
    1095 | TEST_F_FORK(layout1, interleaved_masked_accesses)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1095:1: note: in expansion of macro 'TEST_F_FORK'
    1095 | TEST_F_FORK(layout1, interleaved_masked_accesses)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1095:1: note: in expansion of macro 'TEST_F_FORK'
    1095 | TEST_F_FORK(layout1, interleaved_masked_accesses)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_inherit_subset':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1280:1: note: in expansion of macro 'TEST_F_FORK'
    1280 | TEST_F_FORK(layout1, inherit_subset)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1280:1: note: in expansion of macro 'TEST_F_FORK'
    1280 | TEST_F_FORK(layout1, inherit_subset)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1280:1: note: in expansion of macro 'TEST_F_FORK'
    1280 | TEST_F_FORK(layout1, inherit_subset)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_inherit_superset':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1397:1: note: in expansion of macro 'TEST_F_FORK'
    1397 | TEST_F_FORK(layout1, inherit_superset)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1397:1: note: in expansion of macro 'TEST_F_FORK'
    1397 | TEST_F_FORK(layout1, inherit_superset)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1397:1: note: in expansion of macro 'TEST_F_FORK'
    1397 | TEST_F_FORK(layout1, inherit_superset)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout0_max_layers':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1434:1: note: in expansion of macro 'TEST_F_FORK'
    1434 | TEST_F_FORK(layout0, max_layers)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1434:1: note: in expansion of macro 'TEST_F_FORK'
    1434 | TEST_F_FORK(layout0, max_layers)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1434:1: note: in expansion of macro 'TEST_F_FORK'
    1434 | TEST_F_FORK(layout0, max_layers)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_empty_or_same_ruleset':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1458:1: note: in expansion of macro 'TEST_F_FORK'
    1458 | TEST_F_FORK(layout1, empty_or_same_ruleset)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1458:1: note: in expansion of macro 'TEST_F_FORK'
    1458 | TEST_F_FORK(layout1, empty_or_same_ruleset)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1458:1: note: in expansion of macro 'TEST_F_FORK'
    1458 | TEST_F_FORK(layout1, empty_or_same_ruleset)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_rule_on_mountpoint':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1492:1: note: in expansion of macro 'TEST_F_FORK'
    1492 | TEST_F_FORK(layout1, rule_on_mountpoint)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1492:1: note: in expansion of macro 'TEST_F_FORK'
    1492 | TEST_F_FORK(layout1, rule_on_mountpoint)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1492:1: note: in expansion of macro 'TEST_F_FORK'
    1492 | TEST_F_FORK(layout1, rule_on_mountpoint)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_rule_over_mountpoint':
>> common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1521:1: note: in expansion of macro 'TEST_F_FORK'
    1521 | TEST_F_FORK(layout1, rule_over_mountpoint)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1521:1: note: in expansion of macro 'TEST_F_FORK'
    1521 | TEST_F_FORK(layout1, rule_over_mountpoint)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1521:1: note: in expansion of macro 'TEST_F_FORK'
    1521 | TEST_F_FORK(layout1, rule_over_mountpoint)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_rule_over_root_allow_then_deny':
   common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1554:1: note: in expansion of macro 'TEST_F_FORK'
    1554 | TEST_F_FORK(layout1, rule_over_root_allow_then_deny)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1554:1: note: in expansion of macro 'TEST_F_FORK'
    1554 | TEST_F_FORK(layout1, rule_over_root_allow_then_deny)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1554:1: note: in expansion of macro 'TEST_F_FORK'
    1554 | TEST_F_FORK(layout1, rule_over_root_allow_then_deny)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_rule_over_root_deny':
   common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1584:1: note: in expansion of macro 'TEST_F_FORK'
    1584 | TEST_F_FORK(layout1, rule_over_root_deny)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1584:1: note: in expansion of macro 'TEST_F_FORK'
    1584 | TEST_F_FORK(layout1, rule_over_root_deny)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1584:1: note: in expansion of macro 'TEST_F_FORK'
    1584 | TEST_F_FORK(layout1, rule_over_root_deny)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_rule_inside_mount_ns':
   common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1604:1: note: in expansion of macro 'TEST_F_FORK'
    1604 | TEST_F_FORK(layout1, rule_inside_mount_ns)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1604:1: note: in expansion of macro 'TEST_F_FORK'
    1604 | TEST_F_FORK(layout1, rule_inside_mount_ns)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1604:1: note: in expansion of macro 'TEST_F_FORK'
    1604 | TEST_F_FORK(layout1, rule_inside_mount_ns)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_mount_and_pivot':
   common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1632:1: note: in expansion of macro 'TEST_F_FORK'
    1632 | TEST_F_FORK(layout1, mount_and_pivot)
         | ^~~~~~~~~~~
   common.h:58:34: error: 'struct __test_metadata' has no member named 'step'
      58 |                         _metadata->step = 1; \
         |                                  ^~
   fs_test.c:1632:1: note: in expansion of macro 'TEST_F_FORK'
    1632 | TEST_F_FORK(layout1, mount_and_pivot)
         | ^~~~~~~~~~~
   common.h:71:34: error: 'struct __test_metadata' has no member named 'step'
      71 |                         _metadata->step = WEXITSTATUS(status); \
         |                                  ^~
   fs_test.c:1632:1: note: in expansion of macro 'TEST_F_FORK'
    1632 | TEST_F_FORK(layout1, mount_and_pivot)
         | ^~~~~~~~~~~
   fs_test.c: In function 'layout1_move_mount':
   common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
      52 |                         _exit(_metadata->step); \
         |                                        ^~
   fs_test.c:1655:1: note: in expansion of macro 'TEST_F_FORK'
    1655 | TEST_F_FORK(layout1, move_mount)
         | ^~~~~~~~~~~

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next 2/4] selftests: kselftest_harness: use KSFT_* exit codes

[Jakub Kicinski]

Now that we no longer need low exit codes to communicate
assertion steps - use normal KSFT exit codes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 98bdedf9a53a..618b41eac749 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -874,7 +874,7 @@ static inline void __test_check_assert(struct __test_metadata *t)
 {
 	if (t->aborted) {
 		if (t->no_print)
-			_exit(1);
+			_exit(KSFT_FAIL);
 		abort();
 	}
 }
@@ -937,7 +937,7 @@ void __wait_for_test(struct __test_metadata *t)
 		fprintf(TH_LOG_STREAM,
 			"# %s: Test terminated by timeout\n", t->name);
 	} else if (WIFEXITED(status)) {
-		if (WEXITSTATUS(status) == 255) {
+		if (WEXITSTATUS(status) == KSFT_SKIP) {
 			/* SKIP */
 			t->passed = 1;
 			t->skip = 1;
@@ -950,7 +950,7 @@ void __wait_for_test(struct __test_metadata *t)
 		} else {
 			switch (WEXITSTATUS(status)) {
 			/* Success */
-			case 0:
+			case KSFT_PASS:
 				t->passed = 1;
 				break;
 			/* Other failure, assume step report. */
@@ -1132,12 +1132,11 @@ void __run_test(struct __fixture_metadata *f,
 		setpgrp();
 		t->fn(t, variant);
 		if (t->skip)
-			_exit(255);
-		/* Pass is exit 0 */
+			_exit(KSFT_SKIP);
 		if (t->passed)
-			_exit(0);
+			_exit(KSFT_PASS);
 		/* Something else happened. */
-		_exit(1);
+		_exit(KSFT_FAIL);
 	} else {
 		__wait_for_test(t);
 	}
-- 
2.43.0

Re: [PATCH net-next 2/4] selftests: kselftest_harness: use KSFT_* exit codes

[Kees Cook]

On Tue, Feb 13, 2024 at 07:44:14AM -0800, Jakub Kicinski wrote:
> Now that we no longer need low exit codes to communicate
> assertion steps - use normal KSFT exit codes.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Yeah, good cleanup.

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

[PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

Selftest summary includes XFAIL but there's no way to use
it from within the harness. Support it in a similar way to skip.

Currently tests report skip for things they expect to fail
e.g. when given combination of parameters is known to be unsupported.
This is confusing because in an ideal environment and fully featured
kernel no tests should be skipped.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 37 +++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 618b41eac749..561a817117f9 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -141,6 +141,33 @@
 	statement; \
 } while (0)
 
+/**
+ * XFAIL()
+ *
+ * @statement: statement to run after reporting XFAIL
+ * @fmt: format string
+ * @...: optional arguments
+ *
+ * .. code-block:: c
+ *
+ *     XFAIL(statement, fmt, ...);
+ *
+ * This forces a "pass" after reporting why something is expected to fail,
+ * and runs "statement", which is usually "return" or "goto skip".
+ */
+#define XFAIL(statement, fmt, ...) do { \
+	snprintf(_metadata->results->reason, \
+		 sizeof(_metadata->results->reason), fmt, ##__VA_ARGS__); \
+	if (TH_LOG_ENABLED) { \
+		fprintf(TH_LOG_STREAM, "#      XFAIL      %s\n", \
+			_metadata->results->reason); \
+	} \
+	_metadata->passed = 1; \
+	_metadata->xfail = 1; \
+	_metadata->trigger = 0; \
+	statement; \
+} while (0)
+
 /**
  * TEST() - Defines the test function and creates the registration
  * stub
@@ -834,6 +861,7 @@ struct __test_metadata {
 	int termsig;
 	int passed;
 	int skip;	/* did SKIP get used? */
+	int xfail;	/* did XFAIL get used? */
 	int trigger; /* extra handler after the evaluation */
 	int timeout;	/* seconds to wait for test timeout */
 	bool timed_out;	/* did this test timeout instead of exiting? */
@@ -941,6 +969,9 @@ void __wait_for_test(struct __test_metadata *t)
 			/* SKIP */
 			t->passed = 1;
 			t->skip = 1;
+		} else if (WEXITSTATUS(status) == KSFT_XFAIL) {
+			t->passed = 1;
+			t->xfail = 1;
 		} else if (t->termsig != -1) {
 			t->passed = 0;
 			fprintf(TH_LOG_STREAM,
@@ -1112,6 +1143,7 @@ void __run_test(struct __fixture_metadata *f,
 	/* reset test struct */
 	t->passed = 1;
 	t->skip = 0;
+	t->xfail = 0;
 	t->trigger = 0;
 	t->no_print = 0;
 	memset(t->results->reason, 0, sizeof(t->results->reason));
@@ -1133,6 +1165,8 @@ void __run_test(struct __fixture_metadata *f,
 		t->fn(t, variant);
 		if (t->skip)
 			_exit(KSFT_SKIP);
+		if (t->xfail)
+			_exit(KSFT_XFAIL);
 		if (t->passed)
 			_exit(KSFT_PASS);
 		/* Something else happened. */
@@ -1146,6 +1180,9 @@ void __run_test(struct __fixture_metadata *f,
 	if (t->skip)
 		ksft_test_result_skip("%s\n", t->results->reason[0] ?
 					t->results->reason : "unknown");
+	else if (t->xfail)
+		ksft_test_result_xfail("%s\n", t->results->reason[0] ?
+				       t->results->reason : "unknown");
 	else
 		ksft_test_result(t->passed, "%s%s%s.%s\n",
 			f->name, variant->name[0] ? "." : "", variant->name, t->name);
-- 
2.43.0

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Kees Cook]

On Tue, Feb 13, 2024 at 07:44:15AM -0800, Jakub Kicinski wrote:
> Selftest summary includes XFAIL but there's no way to use
> it from within the harness. Support it in a similar way to skip.
> 
> Currently tests report skip for things they expect to fail
> e.g. when given combination of parameters is known to be unsupported.
> This is confusing because in an ideal environment and fully featured
> kernel no tests should be skipped.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

This is great -- I've been wanting an XFAIL for a while for harness
users but I didn't have the imagination to find a way to make it work.
:)

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Jakub Sitnicki]

On Tue, Feb 13, 2024 at 07:44 AM -08, Jakub Kicinski wrote:
> Selftest summary includes XFAIL but there's no way to use
> it from within the harness. Support it in a similar way to skip.
>
> Currently tests report skip for things they expect to fail
> e.g. when given combination of parameters is known to be unsupported.
> This is confusing because in an ideal environment and fully featured
> kernel no tests should be skipped.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/kselftest_harness.h | 37 +++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 618b41eac749..561a817117f9 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -141,6 +141,33 @@
>  	statement; \
>  } while (0)
>  
> +/**
> + * XFAIL()
> + *
> + * @statement: statement to run after reporting XFAIL
> + * @fmt: format string
> + * @...: optional arguments
> + *
> + * .. code-block:: c
> + *
> + *     XFAIL(statement, fmt, ...);
> + *
> + * This forces a "pass" after reporting why something is expected to fail,
> + * and runs "statement", which is usually "return" or "goto skip".
> + */
> +#define XFAIL(statement, fmt, ...) do { \
> +	snprintf(_metadata->results->reason, \
> +		 sizeof(_metadata->results->reason), fmt, ##__VA_ARGS__); \
> +	if (TH_LOG_ENABLED) { \
> +		fprintf(TH_LOG_STREAM, "#      XFAIL      %s\n", \
> +			_metadata->results->reason); \
> +	} \
> +	_metadata->passed = 1; \
> +	_metadata->xfail = 1; \
> +	_metadata->trigger = 0; \
> +	statement; \
> +} while (0)
> +
>  /**
>   * TEST() - Defines the test function and creates the registration
>   * stub
> @@ -834,6 +861,7 @@ struct __test_metadata {
>  	int termsig;
>  	int passed;
>  	int skip;	/* did SKIP get used? */
> +	int xfail;	/* did XFAIL get used? */
>  	int trigger; /* extra handler after the evaluation */
>  	int timeout;	/* seconds to wait for test timeout */
>  	bool timed_out;	/* did this test timeout instead of exiting? */
> @@ -941,6 +969,9 @@ void __wait_for_test(struct __test_metadata *t)
>  			/* SKIP */
>  			t->passed = 1;
>  			t->skip = 1;
> +		} else if (WEXITSTATUS(status) == KSFT_XFAIL) {
> +			t->passed = 1;
> +			t->xfail = 1;
>  		} else if (t->termsig != -1) {
>  			t->passed = 0;
>  			fprintf(TH_LOG_STREAM,
> @@ -1112,6 +1143,7 @@ void __run_test(struct __fixture_metadata *f,
>  	/* reset test struct */
>  	t->passed = 1;
>  	t->skip = 0;
> +	t->xfail = 0;
>  	t->trigger = 0;
>  	t->no_print = 0;
>  	memset(t->results->reason, 0, sizeof(t->results->reason));
> @@ -1133,6 +1165,8 @@ void __run_test(struct __fixture_metadata *f,
>  		t->fn(t, variant);
>  		if (t->skip)
>  			_exit(KSFT_SKIP);
> +		if (t->xfail)
> +			_exit(KSFT_XFAIL);
>  		if (t->passed)
>  			_exit(KSFT_PASS);
>  		/* Something else happened. */
> @@ -1146,6 +1180,9 @@ void __run_test(struct __fixture_metadata *f,
>  	if (t->skip)
>  		ksft_test_result_skip("%s\n", t->results->reason[0] ?
>  					t->results->reason : "unknown");
> +	else if (t->xfail)
> +		ksft_test_result_xfail("%s\n", t->results->reason[0] ?
> +				       t->results->reason : "unknown");
>  	else
>  		ksft_test_result(t->passed, "%s%s%s.%s\n",
>  			f->name, variant->name[0] ? "." : "", variant->name, t->name);

On second thought, if I can suggest a follow up change so this:

ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT

... becomes this

ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT

You see, we parse test results if they are in TAP format. Lack of test
name for xfail'ed and skip'ed tests makes it difficult to report in CI
which subtest was it. Happy to contribute it, once this series gets
applied.

A quick 'n dirty change could look like below. Open to better ideas.

---8<---

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index a781e6311810..b73985df9cb9 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -211,7 +211,8 @@ static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...)
 		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
 	} while (0)
 
-static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
+static inline __printf(2, 3) void ksft_test_result_xfail(const char *test_name,
+							 const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -219,7 +220,7 @@ static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
 	ksft_cnt.ksft_xfail++;
 
 	va_start(args, msg);
-	printf("ok %u # XFAIL ", ksft_test_num());
+	printf("ok %u %s # XFAIL ", ksft_test_num(), test_name);
 	errno = saved_errno;
 	vprintf(msg, args);
 	va_end(args);
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 561a817117f9..2db647f98abc 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -56,6 +56,7 @@
 #include <asm/types.h>
 #include <ctype.h>
 #include <errno.h>
+#include <limits.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -1140,6 +1141,8 @@ void __run_test(struct __fixture_metadata *f,
 		struct __fixture_variant_metadata *variant,
 		struct __test_metadata *t)
 {
+	char test_name[LINE_MAX];
+
 	/* reset test struct */
 	t->passed = 1;
 	t->skip = 0;
@@ -1149,8 +1152,9 @@ void __run_test(struct __fixture_metadata *f,
 	memset(t->results->reason, 0, sizeof(t->results->reason));
 	t->results->step = 1;
 
-	ksft_print_msg(" RUN           %s%s%s.%s ...\n",
-	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	snprintf(test_name, sizeof(test_name), "%s%s%s.%s",
+		 f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	ksft_print_msg(" RUN           %s ...\n", test_name);
 
 	/* Make sure output buffers are flushed before fork */
 	fflush(stdout);
@@ -1174,18 +1178,16 @@ void __run_test(struct __fixture_metadata *f,
 	} else {
 		__wait_for_test(t);
 	}
-	ksft_print_msg("         %4s  %s%s%s.%s\n", t->passed ? "OK" : "FAIL",
-	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
+	ksft_print_msg("         %4s  %s\n", t->passed ? "OK" : "FAIL", test_name);
 
 	if (t->skip)
 		ksft_test_result_skip("%s\n", t->results->reason[0] ?
 					t->results->reason : "unknown");
 	else if (t->xfail)
-		ksft_test_result_xfail("%s\n", t->results->reason[0] ?
+		ksft_test_result_xfail(test_name, "%s\n", t->results->reason[0] ?
 				       t->results->reason : "unknown");
 	else
-		ksft_test_result(t->passed, "%s%s%s.%s\n",
-			f->name, variant->name[0] ? "." : "", variant->name, t->name);
+		ksft_test_result(t->passed, "%s\n", test_name);
 }
 
 static int test_harness_run(int argc, char **argv)
diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c
index 2f8b991f78cb..0abab3b32c88 100644
--- a/tools/testing/selftests/mm/mremap_test.c
+++ b/tools/testing/selftests/mm/mremap_test.c
@@ -575,8 +575,7 @@ static void run_mremap_test_case(struct test test_case, int *failures,
 
 	if (remap_time < 0) {
 		if (test_case.expect_failure)
-			ksft_test_result_xfail("%s\n\tExpected mremap failure\n",
-					      test_case.name);
+			ksft_test_result_xfail(test_case.name, "\n\tExpected mremap failure\n");
 		else {
 			ksft_test_result_fail("%s\n", test_case.name);
 			*failures += 1;
diff --git a/tools/testing/selftests/net/tcp_ao/key-management.c b/tools/testing/selftests/net/tcp_ao/key-management.c
index 24e62120b792..928f067513da 100644
--- a/tools/testing/selftests/net/tcp_ao/key-management.c
+++ b/tools/testing/selftests/net/tcp_ao/key-management.c
@@ -123,8 +123,8 @@ static void try_delete_key(char *tst_name, int sk, uint8_t sndid, uint8_t rcvid,
 		return;
 	}
 	if (err && fault(FIXME)) {
-		test_xfail("%s: failed to delete the key %u:%u %d",
-			   tst_name, sndid, rcvid, err);
+		test_xfail(tst_name, "failed to delete the key %u:%u %d",
+			   sndid, rcvid, err);
 		return;
 	}
 	if (!err) {
@@ -283,8 +283,7 @@ static void assert_no_current_rnext(const char *tst_msg, int sk)
 
 	errno = 0;
 	if (ao_info.set_current || ao_info.set_rnext) {
-		test_xfail("%s: the socket has current/rnext keys: %d:%d",
-			   tst_msg,
+		test_xfail(tst_msg, "the socket has current/rnext keys: %d:%d",
 			   (ao_info.set_current) ? ao_info.current_key : -1,
 			   (ao_info.set_rnext) ? ao_info.rnext : -1);
 	} else {
diff --git a/tools/testing/selftests/net/tcp_ao/lib/aolib.h b/tools/testing/selftests/net/tcp_ao/lib/aolib.h
index fbc7f6111815..0d6f33b51758 100644
--- a/tools/testing/selftests/net/tcp_ao/lib/aolib.h
+++ b/tools/testing/selftests/net/tcp_ao/lib/aolib.h
@@ -59,8 +59,8 @@ static inline void __test_print(void (*fn)(const char *), const char *fmt, ...)
 	__test_print(__test_ok, fmt "\n", ##__VA_ARGS__)
 #define test_skip(fmt, ...)						\
 	__test_print(__test_skip, fmt "\n", ##__VA_ARGS__)
-#define test_xfail(fmt, ...)						\
-	__test_print(__test_xfail, fmt "\n", ##__VA_ARGS__)
+#define test_xfail(test_name, fmt, ...)					\
+	__test_print(__test_xfail, test_name, fmt "\n", ##__VA_ARGS__)
 
 #define test_fail(fmt, ...)						\
 do {									\

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Jakub Sitnicki]

On Wed, Feb 14, 2024 at 08:40 PM +01, Jakub Sitnicki wrote:

[...]

> On second thought, if I can suggest a follow up change so this:
>
> ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
>
> ... becomes this
>
> ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
>
> You see, we parse test results if they are in TAP format. Lack of test
> name for xfail'ed and skip'ed tests makes it difficult to report in CI
> which subtest was it. Happy to contribute it, once this series gets
> applied.

Should have said "harder", not "difficult". That was an overstatement.

Test name can be extracted from diagnostic lines preceeding the status.

#  RUN           ip_local_port_range.ip4_stcp.late_bind ...
#      XFAIL      SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
#            OK  ip_local_port_range.ip4_stcp.late_bind
ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT

It just makes the TAP parser easier if the test name is included on the
status line. That would be the motivation here. Let me know what you
think.

[...]

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

On Wed, 14 Feb 2024 22:46:46 +0100 Jakub Sitnicki wrote:
> > On second thought, if I can suggest a follow up change so this:
> >
> > ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> >
> > ... becomes this
> >
> > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> >
> > You see, we parse test results if they are in TAP format. Lack of test
> > name for xfail'ed and skip'ed tests makes it difficult to report in CI
> > which subtest was it. Happy to contribute it, once this series gets
> > applied.  
> 
> Should have said "harder", not "difficult". That was an overstatement.
> 
> Test name can be extracted from diagnostic lines preceeding the status.
> 
> #  RUN           ip_local_port_range.ip4_stcp.late_bind ...
> #      XFAIL      SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> #            OK  ip_local_port_range.ip4_stcp.late_bind
> ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> 
> It just makes the TAP parser easier if the test name is included on the
> status line. That would be the motivation here. Let me know what you
> think.

Good catch, I just copied what we do for skip and completely missed
this. As you said we'd report:

ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT

and I think that's sort of closer to valid TAP than to valid KTAP
which always mentions test/test_case_name:

https://docs.kernel.org/dev-tools/ktap.html

We currently do the same thing for SKIP, e.g.:

#  RUN           ip_local_port_range.ip4_stcp.late_bind ...
#      SKIP      SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
#            OK  ip_local_port_range.ip4_stcp.late_bind
ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT

I'm not sure if we can realistically do surgery on the existing print
helpers to add the test_name, because:

$ git grep 'ksft_test_result_*' | wc -l
915

That'd be a cruel patch to send.

But I do agree that adding the test_name to the prototype is a good
move, to avoid others making the same mistake. Should we introduce
a new set of helpers which take the extra arg and call them
ksft_test_report_*() instead of ksft_test_result_*() ?

Maybe we're overthinking and a local fix in the harness is enough.

Kees, WDYT?

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Kees Cook]

On Wed, Feb 14, 2024 at 04:25:14PM -0800, Jakub Kicinski wrote:
> On Wed, 14 Feb 2024 22:46:46 +0100 Jakub Sitnicki wrote:
> > > On second thought, if I can suggest a follow up change so this:
> > >
> > > ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> > >
> > > ... becomes this
> > >
> > > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> > >
> > > You see, we parse test results if they are in TAP format. Lack of test
> > > name for xfail'ed and skip'ed tests makes it difficult to report in CI
> > > which subtest was it. Happy to contribute it, once this series gets
> > > applied.  
> > 
> > Should have said "harder", not "difficult". That was an overstatement.
> > 
> > Test name can be extracted from diagnostic lines preceeding the status.
> > 
> > #  RUN           ip_local_port_range.ip4_stcp.late_bind ...
> > #      XFAIL      SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> > #            OK  ip_local_port_range.ip4_stcp.late_bind
> > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> > 
> > It just makes the TAP parser easier if the test name is included on the
> > status line. That would be the motivation here. Let me know what you
> > think.
> 
> Good catch, I just copied what we do for skip and completely missed
> this. As you said we'd report:
> 
> ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> 
> and I think that's sort of closer to valid TAP than to valid KTAP
> which always mentions test/test_case_name:
> 
> https://docs.kernel.org/dev-tools/ktap.html
> 
> We currently do the same thing for SKIP, e.g.:
> 
> #  RUN           ip_local_port_range.ip4_stcp.late_bind ...
> #      SKIP      SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> #            OK  ip_local_port_range.ip4_stcp.late_bind
> ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
> 
> I'm not sure if we can realistically do surgery on the existing print
> helpers to add the test_name, because:
> 
> $ git grep 'ksft_test_result_*' | wc -l
> 915
> 
> That'd be a cruel patch to send.
> 
> But I do agree that adding the test_name to the prototype is a good
> move, to avoid others making the same mistake. Should we introduce
> a new set of helpers which take the extra arg and call them
> ksft_test_report_*() instead of ksft_test_result_*() ?
> 
> Maybe we're overthinking and a local fix in the harness is enough.
> 
> Kees, WDYT?

Yeah, let's separate this fix-up from the addition of the XFAIL logic.

-- 
Kees Cook

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Kees Cook]

On Tue, Feb 13, 2024 at 07:44:15AM -0800, Jakub Kicinski wrote:
> [...]
> +/**
> + * XFAIL()
> + *
> + * @statement: statement to run after reporting XFAIL
> + * @fmt: format string
> + * @...: optional arguments
> + *
> + * .. code-block:: c
> + *
> + *     XFAIL(statement, fmt, ...);
> + *
> + * This forces a "pass" after reporting why something is expected to fail,
> + * and runs "statement", which is usually "return" or "goto skip".
> + */
> +#define XFAIL(statement, fmt, ...) do { \
> +	snprintf(_metadata->results->reason, \
> +		 sizeof(_metadata->results->reason), fmt, ##__VA_ARGS__); \
> +	if (TH_LOG_ENABLED) { \
> +		fprintf(TH_LOG_STREAM, "#      XFAIL      %s\n", \

Oh! I just noticed this while testing changes to use XFAIL, there is an
alignment issue: one too many spaces after "XFAIL" above, which leads
to misaligned output.

		fprintf(TH_LOG_STREAM, "#      XFAIL      %s\n", \
                fprintf(TH_LOG_STREAM, "#      SKIP      %s\n", \

Compare the position of "%s" above...

-- 
Kees Cook

Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Jakub Kicinski]

On Thu, 15 Feb 2024 14:06:58 -0800 Kees Cook wrote:
> Oh! I just noticed this while testing changes to use XFAIL, there is an
> alignment issue: one too many spaces after "XFAIL" above, which leads
> to misaligned output.
> 
> 		fprintf(TH_LOG_STREAM, "#      XFAIL      %s\n", \
>                 fprintf(TH_LOG_STREAM, "#      SKIP      %s\n", \
> 
> Compare the position of "%s" above...

Ah, sadness. Thanks for catching. Let me fix, look this over again,
and perhaps add the new helpers Jakub S suggested.

[PATCH net-next 4/4] selftests: ip_local_port_range: use XFAIL instead of SKIP

[Jakub Kicinski]

SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
so use XFAIL instead of SKIP.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/ip_local_port_range.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c
index 0f217a1cc837..c1d3f001c240 100644
--- a/tools/testing/selftests/net/ip_local_port_range.c
+++ b/tools/testing/selftests/net/ip_local_port_range.c
@@ -362,7 +362,7 @@ TEST_F(ip_local_port_range, late_bind)
 	__u16 port;
 
 	if (variant->so_protocol == IPPROTO_SCTP)
-		SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
+		XFAIL(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
 
 	fd = socket(variant->so_domain, variant->so_type, 0);
 	ASSERT_GE(fd, 0) TH_LOG("socket failed");
-- 
2.43.0

Re: [PATCH net-next 4/4] selftests: ip_local_port_range: use XFAIL instead of SKIP

[Kees Cook]

On Tue, Feb 13, 2024 at 07:44:16AM -0800, Jakub Kicinski wrote:
> SCTP does not support IP_LOCAL_PORT_RANGE and we know it,
> so use XFAIL instead of SKIP.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH net-next 0/4] selftests: kselftest_harness: support using xfail

[Kees Cook]

On Tue, Feb 13, 2024 at 07:44:12AM -0800, Jakub Kicinski wrote:
> Hi!
> 
> When running selftests for our subsystem in our CI we'd like all
> tests to pass. Currently some tests use SKIP for cases they
> expect to fail, because the kselftest_harness limits the return
> codes to pass/fail/skip.
> 
> Clean up and support the use of the full range of ksft exit codes
> under kselftest_harness.
> 
> To avoid conflicts and get the functionality into the networking
> tree ASAP I'd like to put the patches on shared branch so that
> both linux-kselftest and net-next can pull it in. Shuah, please
> LMK if that'd work for you, and if so which -rc should I base
> the branch on. Or is merging directly into net-next okay?

I would use XFAIL for seccomp selftests too, but I can wait for the next
release. i.e. I don't need a shared branch -- it'd be fine in net-next.
But I defer to Shuah as far as the selftest tree is concerned. (FWIW, I
don't see any current conflicts.)

-- 
Kees Cook

Re: [PATCH net-next 0/4] selftests: kselftest_harness: support using xfail

[Jakub Sitnicki]

On Tue, Feb 13, 2024 at 07:44 AM -08, Jakub Kicinski wrote:
> Hi!
>
> When running selftests for our subsystem in our CI we'd like all
> tests to pass. Currently some tests use SKIP for cases they
> expect to fail, because the kselftest_harness limits the return
> codes to pass/fail/skip.
>
> Clean up and support the use of the full range of ksft exit codes
> under kselftest_harness.
>
> To avoid conflicts and get the functionality into the networking
> tree ASAP I'd like to put the patches on shared branch so that
> both linux-kselftest and net-next can pull it in. Shuah, please
> LMK if that'd work for you, and if so which -rc should I base
> the branch on. Or is merging directly into net-next okay?
>
> Jakub Kicinski (4):
>   selftests: kselftest_harness: pass step via shared memory
>   selftests: kselftest_harness: use KSFT_* exit codes
>   selftests: kselftest_harness: support using xfail
>   selftests: ip_local_port_range: use XFAIL instead of SKIP
>
>  tools/testing/selftests/kselftest_harness.h   | 67 ++++++++++++++-----
>  .../selftests/net/ip_local_port_range.c       |  2 +-
>  2 files changed, 52 insertions(+), 17 deletions(-)

Nice!
We've been ignoring skipped tests in our internal CI.
But this is the wrong approach, as you point out.

For the series:

Tested-by: Jakub Sitnicki <jakub@cloudflare.com>