[PATCH v3 0/6] kselftest: add fixture parameters

[PATCH v3 0/6] kselftest: add fixture parameters

[Jakub Kicinski]

Hi!

Shuah please consider applying to the kselftest tree.

This set is an attempt to make running tests for different
sets of data easier. The direct motivation is the tls
test which we'd like to run for TLS 1.2 and TLS 1.3,
but currently there is no easy way to invoke the same
tests with different parameters.

Tested all users of kselftest_harness.h.

v2:
 - don't run tests by fixture
 - don't pass params as an explicit argument

v3:
 - go back to the orginal implementation with an extra
   parameter, and running by fixture (Kees);
 - add LIST_APPEND helper (Kees);
 - add a dot between fixture and param name (Kees);
 - rename the params to variants (Tim);

v1: https://lore.kernel.org/netdev/20200313031752.2332565-1-kuba@kernel.org/
v2: https://lore.kernel.org/netdev/20200314005501.2446494-1-kuba@kernel.org/

Jakub Kicinski (6):
  selftests/seccomp: use correct FIXTURE macro
  kselftest: factor out list manipulation to a helper
  kselftest: create fixture objects
  kselftest: run tests by fixture
  kselftest: add fixture variants
  selftests: tls: run all tests for TLS 1.2 and TLS 1.3

 Documentation/dev-tools/kselftest.rst         |   3 +-
 tools/testing/selftests/kselftest_harness.h   | 233 ++++++++++++++----
 tools/testing/selftests/net/tls.c             |  93 ++-----
 tools/testing/selftests/seccomp/seccomp_bpf.c |  10 +-
 4 files changed, 206 insertions(+), 133 deletions(-)

-- 
2.24.1

[PATCH v3 1/6] selftests/seccomp: use correct FIXTURE macro

[Jakub Kicinski]

Quoting kdoc:

FIXTURE_DATA:
 * This call may be used when the type of the fixture data
 * is needed.  In general, this should not be needed unless
 * the *self* is being passed to a helper directly.

FIXTURE:
 * Defines the data provided to TEST_F()-defined tests as *self*.  It should be
 * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().

seccomp should use FIXTURE to declare types.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index ee1b727ede04..7bf82fb07f67 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -909,7 +909,7 @@ TEST(ERRNO_order)
 	EXPECT_EQ(12, errno);
 }
 
-FIXTURE_DATA(TRAP) {
+FIXTURE(TRAP) {
 	struct sock_fprog prog;
 };
 
@@ -1020,7 +1020,7 @@ TEST_F(TRAP, handler)
 	EXPECT_NE(0, (unsigned long)sigsys->_call_addr);
 }
 
-FIXTURE_DATA(precedence) {
+FIXTURE(precedence) {
 	struct sock_fprog allow;
 	struct sock_fprog log;
 	struct sock_fprog trace;
@@ -1509,7 +1509,7 @@ void tracer_poke(struct __test_metadata *_metadata, pid_t tracee, int status,
 	EXPECT_EQ(0, ret);
 }
 
-FIXTURE_DATA(TRACE_poke) {
+FIXTURE(TRACE_poke) {
 	struct sock_fprog prog;
 	pid_t tracer;
 	long poked;
@@ -1817,7 +1817,7 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
 		change_syscall(_metadata, tracee, -1, -ESRCH);
 }
 
-FIXTURE_DATA(TRACE_syscall) {
+FIXTURE(TRACE_syscall) {
 	struct sock_fprog prog;
 	pid_t tracer, mytid, mypid, parent;
 };
@@ -2321,7 +2321,7 @@ struct tsync_sibling {
 		}							\
 	} while (0)
 
-FIXTURE_DATA(TSYNC) {
+FIXTURE(TSYNC) {
 	struct sock_fprog root_prog, apply_prog;
 	struct tsync_sibling sibling[TSYNC_SIBLINGS];
 	sem_t started;
-- 
2.24.1

[PATCH v3 2/6] kselftest: factor out list manipulation to a helper

[Jakub Kicinski]

Kees suggest to factor out the list append code to a macro,
since following commits need it, which leads to code duplication.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 42 ++++++++++++---------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5336b26506ab..aaf58fffc8f7 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -631,6 +631,29 @@
 	} \
 } while (0); OPTIONAL_HANDLER(_assert)
 
+/* List helpers */
+#define __LIST_APPEND(head, item) \
+{ \
+	/* Circular linked list where only prev is circular. */ \
+	if (head == NULL) { \
+		head = item; \
+		item->next = NULL; \
+		item->prev = item; \
+		return;	\
+	} \
+	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) { \
+		item->next = NULL; \
+		item->prev = head->prev; \
+		item->prev->next = item; \
+		head->prev = item; \
+	} else { \
+		item->next = head; \
+		item->next->prev = item; \
+		item->prev = item; \
+		head = item; \
+	} \
+}
+
 /* Contains all the information for test execution and status checking. */
 struct __test_metadata {
 	const char *name;
@@ -665,24 +688,7 @@ static int __constructor_order;
 static inline void __register_test(struct __test_metadata *t)
 {
 	__test_count++;
-	/* Circular linked list where only prev is circular. */
-	if (__test_list == NULL) {
-		__test_list = t;
-		t->next = NULL;
-		t->prev = t;
-		return;
-	}
-	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
-		t->next = NULL;
-		t->prev = __test_list->prev;
-		t->prev->next = t;
-		__test_list->prev = t;
-	} else {
-		t->next = __test_list;
-		t->next->prev = t;
-		t->prev = t;
-		__test_list = t;
-	}
+	__LIST_APPEND(__test_list, t);
 }
 
 static inline int __bail(int for_realz, bool no_print, __u8 step)
-- 
2.24.1

[PATCH v3 3/6] kselftest: create fixture objects

[Jakub Kicinski]

Grouping tests by fixture will allow us to parametrize
test runs. Create full objects for fixtures.

Add a "global" fixture for tests without a fixture.

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

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index aaf58fffc8f7..0f68943d6f04 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -169,8 +169,10 @@
 #define __TEST_IMPL(test_name, _signal) \
 	static void test_name(struct __test_metadata *_metadata); \
 	static struct __test_metadata _##test_name##_object = \
-		{ .name = "global." #test_name, \
-		  .fn = &test_name, .termsig = _signal, \
+		{ .name = #test_name, \
+		  .fn = &test_name, \
+		  .fixture = &_fixture_global, \
+		  .termsig = _signal, \
 		  .timeout = TEST_TIMEOUT_DEFAULT, }; \
 	static void __attribute__((constructor)) _register_##test_name(void) \
 	{ \
@@ -212,10 +214,12 @@
  * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
  */
 #define FIXTURE(fixture_name) \
+	static struct __fixture_metadata _##fixture_name##_fixture_object = \
+		{ .name =  #fixture_name, }; \
 	static void __attribute__((constructor)) \
 	_register_##fixture_name##_data(void) \
 	{ \
-		__fixture_count++; \
+		__register_fixture(&_##fixture_name##_fixture_object); \
 	} \
 	FIXTURE_DATA(fixture_name)
 
@@ -309,8 +313,9 @@
 	} \
 	static struct __test_metadata \
 		      _##fixture_name##_##test_name##_object = { \
-		.name = #fixture_name "." #test_name, \
+		.name = #test_name, \
 		.fn = &wrapper_##fixture_name##_##test_name, \
+		.fixture = &_##fixture_name##_fixture_object, \
 		.termsig = signal, \
 		.timeout = tmout, \
 	 }; \
@@ -654,10 +659,33 @@
 	} \
 }
 
+/* Contains all the information about a fixture */
+struct __fixture_metadata {
+	const char *name;
+	struct __fixture_metadata *prev, *next;
+} _fixture_global __attribute__((unused)) = {
+	.name = "global",
+	.prev = &_fixture_global,
+};
+
+static struct __fixture_metadata *__fixture_list = &_fixture_global;
+static unsigned int __fixture_count;
+static int __constructor_order;
+
+#define _CONSTRUCTOR_ORDER_FORWARD   1
+#define _CONSTRUCTOR_ORDER_BACKWARD -1
+
+static inline void __register_fixture(struct __fixture_metadata *f)
+{
+	__fixture_count++;
+	__LIST_APPEND(__fixture_list, f);
+}
+
 /* Contains all the information for test execution and status checking. */
 struct __test_metadata {
 	const char *name;
 	void (*fn)(struct __test_metadata *);
+	struct __fixture_metadata *fixture;
 	int termsig;
 	int passed;
 	int trigger; /* extra handler after the evaluation */
@@ -670,11 +698,6 @@ struct __test_metadata {
 /* Storage for the (global) tests to be run. */
 static struct __test_metadata *__test_list;
 static unsigned int __test_count;
-static unsigned int __fixture_count;
-static int __constructor_order;
-
-#define _CONSTRUCTOR_ORDER_FORWARD   1
-#define _CONSTRUCTOR_ORDER_BACKWARD -1
 
 /*
  * Since constructors are called in reverse order, reverse the test
@@ -708,7 +731,7 @@ void __run_test(struct __test_metadata *t)
 
 	t->passed = 1;
 	t->trigger = 0;
-	printf("[ RUN      ] %s\n", t->name);
+	printf("[ RUN      ] %s.%s\n", t->fixture->name, t->name);
 	alarm(t->timeout);
 	child_pid = fork();
 	if (child_pid < 0) {
@@ -757,7 +780,8 @@ void __run_test(struct __test_metadata *t)
 				status);
 		}
 	}
-	printf("[     %4s ] %s\n", (t->passed ? "OK" : "FAIL"), t->name);
+	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
+	       t->fixture->name, t->name);
 	alarm(0);
 }
 
-- 
2.24.1

[PATCH v3 4/6] kselftest: run tests by fixture

[Jakub Kicinski]

Now that all tests have a fixture object move from a global
list of tests to a list of tests per fixture.

Order of tests may change as we will now group and run test
fixture by fixture, rather than in declaration order.

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

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 0f68943d6f04..36ab1b92eb35 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -660,8 +660,11 @@
 }
 
 /* Contains all the information about a fixture */
+struct __test_metadata;
+
 struct __fixture_metadata {
 	const char *name;
+	struct __test_metadata *tests;
 	struct __fixture_metadata *prev, *next;
 } _fixture_global __attribute__((unused)) = {
 	.name = "global",
@@ -696,7 +699,6 @@ struct __test_metadata {
 };
 
 /* Storage for the (global) tests to be run. */
-static struct __test_metadata *__test_list;
 static unsigned int __test_count;
 
 /*
@@ -710,8 +712,10 @@ static unsigned int __test_count;
  */
 static inline void __register_test(struct __test_metadata *t)
 {
+	struct __fixture_metadata *f = t->fixture;
+
 	__test_count++;
-	__LIST_APPEND(__test_list, t);
+	__LIST_APPEND(f->tests, t);
 }
 
 static inline int __bail(int for_realz, bool no_print, __u8 step)
@@ -724,14 +728,15 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
 	return 0;
 }
 
-void __run_test(struct __test_metadata *t)
+void __run_test(struct __fixture_metadata *f,
+		struct __test_metadata *t)
 {
 	pid_t child_pid;
 	int status;
 
 	t->passed = 1;
 	t->trigger = 0;
-	printf("[ RUN      ] %s.%s\n", t->fixture->name, t->name);
+	printf("[ RUN      ] %s.%s\n", f->name, t->name);
 	alarm(t->timeout);
 	child_pid = fork();
 	if (child_pid < 0) {
@@ -781,13 +786,14 @@ void __run_test(struct __test_metadata *t)
 		}
 	}
 	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
-	       t->fixture->name, t->name);
+	       f->name, t->name);
 	alarm(0);
 }
 
 static int test_harness_run(int __attribute__((unused)) argc,
 			    char __attribute__((unused)) **argv)
 {
+	struct __fixture_metadata *f;
 	struct __test_metadata *t;
 	int ret = 0;
 	unsigned int count = 0;
@@ -796,13 +802,15 @@ static int test_harness_run(int __attribute__((unused)) argc,
 	/* TODO(wad) add optional arguments similar to gtest. */
 	printf("[==========] Running %u tests from %u test cases.\n",
 	       __test_count, __fixture_count + 1);
-	for (t = __test_list; t; t = t->next) {
-		count++;
-		__run_test(t);
-		if (t->passed)
-			pass_count++;
-		else
-			ret = 1;
+	for (f = __fixture_list; f; f = f->next) {
+		for (t = f->tests; t; t = t->next) {
+			count++;
+			__run_test(f, t);
+			if (t->passed)
+				pass_count++;
+			else
+				ret = 1;
+		}
 	}
 	printf("[==========] %u / %u tests passed.\n", pass_count, count);
 	printf("[  %s  ]\n", (ret ? "FAILED" : "PASSED"));
-- 
2.24.1

[PATCH v3 5/6] kselftest: add fixture parameters

[Jakub Kicinski]

Allow users to pass parameters to fixtures.

Each fixture will be evaluated for each of its parameter
sets.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v3:
 - separate variant name out with a dot.
---
 Documentation/dev-tools/kselftest.rst       |   3 +-
 tools/testing/selftests/kselftest_harness.h | 145 ++++++++++++++++----
 2 files changed, 121 insertions(+), 27 deletions(-)

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index 61ae13c44f91..5d1f56fcd2e7 100644
--- a/Documentation/dev-tools/kselftest.rst
+++ b/Documentation/dev-tools/kselftest.rst
@@ -301,7 +301,8 @@ Helpers
 
 .. kernel-doc:: tools/testing/selftests/kselftest_harness.h
     :functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP
-                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN
+                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN FIXTURE_VARIANT
+                FIXTURE_VARIANT_ADD
 
 Operators
 ---------
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 36ab1b92eb35..1a079afa2d01 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -168,9 +168,15 @@
 
 #define __TEST_IMPL(test_name, _signal) \
 	static void test_name(struct __test_metadata *_metadata); \
+	static inline void wrapper_##test_name( \
+		struct __test_metadata *_metadata, \
+		struct __fixture_variant_metadata *variant) \
+	{ \
+		test_name(_metadata); \
+	} \
 	static struct __test_metadata _##test_name##_object = \
 		{ .name = #test_name, \
-		  .fn = &test_name, \
+		  .fn = &wrapper_##test_name, \
 		  .fixture = &_fixture_global, \
 		  .termsig = _signal, \
 		  .timeout = TEST_TIMEOUT_DEFAULT, }; \
@@ -214,6 +220,7 @@
  * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
  */
 #define FIXTURE(fixture_name) \
+	FIXTURE_VARIANT(fixture_name); \
 	static struct __fixture_metadata _##fixture_name##_fixture_object = \
 		{ .name =  #fixture_name, }; \
 	static void __attribute__((constructor)) \
@@ -245,7 +252,9 @@
 #define FIXTURE_SETUP(fixture_name) \
 	void fixture_name##_setup( \
 		struct __test_metadata __attribute__((unused)) *_metadata, \
-		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
+		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
+		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
+
 /**
  * FIXTURE_TEARDOWN(fixture_name)
  * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
@@ -267,6 +276,58 @@
 		struct __test_metadata __attribute__((unused)) *_metadata, \
 		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
 
+/**
+ * FIXTURE_VARIANT(fixture_name) - Optionally called once per fixture
+ * to declare fixture variant
+ *
+ * @fixture_name: fixture name
+ *
+ * .. code-block:: c
+ *
+ *     FIXTURE_VARIANT(datatype name) {
+ *       type property1;
+ *       ...
+ *     };
+ *
+ * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F()
+ * as *variant*. Variants allow the same tests to be run with different
+ * arguments.
+ */
+#define FIXTURE_VARIANT(fixture_name) struct _fixture_variant_##fixture_name
+
+/**
+ * FIXTURE_VARIANT_ADD(fixture_name, variant_name) - Called once per fixture
+ * variant to setup and register the data
+ *
+ * @fixture_name: fixture name
+ * @variant_name: name of the parameter set
+ *
+ * .. code-block:: c
+ *
+ *     FIXTURE_ADD(datatype name) {
+ *       .property1 = val1;
+ *       ...
+ *     };
+ *
+ * Defines a variant of the test fixture, provided to FIXTURE_SETUP() and
+ * TEST_F() as *variant*. Tests of each fixture will be run once for each
+ * variant.
+ */
+#define FIXTURE_VARIANT_ADD(fixture_name, variant_name) \
+	extern FIXTURE_VARIANT(fixture_name) \
+		_##fixture_name##_##variant_name##_variant; \
+	static struct __fixture_variant_metadata \
+		_##fixture_name##_##variant_name##_object = \
+		{ .name = #variant_name, \
+		  .data = &_##fixture_name##_##variant_name##_variant}; \
+	static void __attribute__((constructor)) \
+		_register_##fixture_name##_##variant_name(void) \
+	{ \
+		__register_fixture_variant(&_##fixture_name##_fixture_object, \
+			&_##fixture_name##_##variant_name##_object);	\
+	} \
+	FIXTURE_VARIANT(fixture_name) _##fixture_name##_##variant_name##_variant =
+
 /**
  * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
  * fixture-based test cases
@@ -297,18 +358,20 @@
 #define __TEST_F_IMPL(fixture_name, test_name, signal, tmout) \
 	static void fixture_name##_##test_name( \
 		struct __test_metadata *_metadata, \
-		FIXTURE_DATA(fixture_name) *self); \
+		FIXTURE_DATA(fixture_name) *self, \
+		const FIXTURE_VARIANT(fixture_name) *variant); \
 	static inline void wrapper_##fixture_name##_##test_name( \
-		struct __test_metadata *_metadata) \
+		struct __test_metadata *_metadata, \
+		struct __fixture_variant_metadata *variant) \
 	{ \
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
-		fixture_name##_setup(_metadata, &self); \
+		fixture_name##_setup(_metadata, &self, variant->data); \
 		/* Let setup failure terminate early. */ \
 		if (!_metadata->passed) \
 			return; \
-		fixture_name##_##test_name(_metadata, &self); \
+		fixture_name##_##test_name(_metadata, &self, variant->data); \
 		fixture_name##_teardown(_metadata, &self); \
 	} \
 	static struct __test_metadata \
@@ -326,7 +389,8 @@
 	} \
 	static void fixture_name##_##test_name( \
 		struct __test_metadata __attribute__((unused)) *_metadata, \
-		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
+		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
+		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
 
 /**
  * TEST_HARNESS_MAIN - Simple wrapper to run the test harness
@@ -661,10 +725,12 @@
 
 /* Contains all the information about a fixture */
 struct __test_metadata;
+struct __fixture_variant_metadata;
 
 struct __fixture_metadata {
 	const char *name;
 	struct __test_metadata *tests;
+	struct __fixture_variant_metadata *variant;
 	struct __fixture_metadata *prev, *next;
 } _fixture_global __attribute__((unused)) = {
 	.name = "global",
@@ -672,7 +738,6 @@ struct __fixture_metadata {
 };
 
 static struct __fixture_metadata *__fixture_list = &_fixture_global;
-static unsigned int __fixture_count;
 static int __constructor_order;
 
 #define _CONSTRUCTOR_ORDER_FORWARD   1
@@ -680,14 +745,27 @@ static int __constructor_order;
 
 static inline void __register_fixture(struct __fixture_metadata *f)
 {
-	__fixture_count++;
 	__LIST_APPEND(__fixture_list, f);
 }
 
+struct __fixture_variant_metadata {
+	const char *name;
+	const void *data;
+	struct __fixture_variant_metadata *prev, *next;
+};
+
+static inline void
+__register_fixture_variant(struct __fixture_metadata *f,
+			  struct __fixture_variant_metadata *variant)
+{
+	__LIST_APPEND(f->variant, variant);
+}
+
 /* Contains all the information for test execution and status checking. */
 struct __test_metadata {
 	const char *name;
-	void (*fn)(struct __test_metadata *);
+	void (*fn)(struct __test_metadata *,
+		   struct __fixture_variant_metadata *);
 	struct __fixture_metadata *fixture;
 	int termsig;
 	int passed;
@@ -698,9 +776,6 @@ struct __test_metadata {
 	struct __test_metadata *prev, *next;
 };
 
-/* Storage for the (global) tests to be run. */
-static unsigned int __test_count;
-
 /*
  * Since constructors are called in reverse order, reverse the test
  * list so tests are run in source declaration order.
@@ -714,7 +789,6 @@ static inline void __register_test(struct __test_metadata *t)
 {
 	struct __fixture_metadata *f = t->fixture;
 
-	__test_count++;
 	__LIST_APPEND(f->tests, t);
 }
 
@@ -729,21 +803,27 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
 }
 
 void __run_test(struct __fixture_metadata *f,
+		struct __fixture_variant_metadata *variant,
 		struct __test_metadata *t)
 {
 	pid_t child_pid;
 	int status;
 
+	/* reset test struct */
 	t->passed = 1;
 	t->trigger = 0;
-	printf("[ RUN      ] %s.%s\n", f->name, t->name);
+	t->step = 0;
+	t->no_print = 0;
+
+	printf("[ RUN      ] %s%s%s.%s\n",
+	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
 	alarm(t->timeout);
 	child_pid = fork();
 	if (child_pid < 0) {
 		printf("ERROR SPAWNING TEST CHILD\n");
 		t->passed = 0;
 	} else if (child_pid == 0) {
-		t->fn(t);
+		t->fn(t, variant);
 		/* return the step that failed or 0 */
 		_exit(t->passed ? 0 : t->step);
 	} else {
@@ -785,31 +865,44 @@ void __run_test(struct __fixture_metadata *f,
 				status);
 		}
 	}
-	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
-	       f->name, t->name);
+	printf("[     %4s ] %s%s%s.%s\n", (t->passed ? "OK" : "FAIL"),
+	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
 	alarm(0);
 }
 
 static int test_harness_run(int __attribute__((unused)) argc,
 			    char __attribute__((unused)) **argv)
 {
+	struct __fixture_variant_metadata no_variant = { .name = "", };
+	struct __fixture_variant_metadata *v;
 	struct __fixture_metadata *f;
 	struct __test_metadata *t;
 	int ret = 0;
+	unsigned int case_count = 0, test_count = 0;
 	unsigned int count = 0;
 	unsigned int pass_count = 0;
 
+	for (f = __fixture_list; f; f = f->next) {
+		for (v = f->variant ?: &no_variant; v; v = v->next) {
+			case_count++;
+			for (t = f->tests; t; t = t->next)
+				test_count++;
+		}
+	}
+
 	/* TODO(wad) add optional arguments similar to gtest. */
 	printf("[==========] Running %u tests from %u test cases.\n",
-	       __test_count, __fixture_count + 1);
+	       test_count, case_count);
 	for (f = __fixture_list; f; f = f->next) {
-		for (t = f->tests; t; t = t->next) {
-			count++;
-			__run_test(f, t);
-			if (t->passed)
-				pass_count++;
-			else
-				ret = 1;
+		for (v = f->variant ?: &no_variant; v; v = v->next) {
+			for (t = f->tests; t; t = t->next) {
+				count++;
+				__run_test(f, v, t);
+				if (t->passed)
+					pass_count++;
+				else
+					ret = 1;
+			}
 		}
 	}
 	printf("[==========] %u / %u tests passed.\n", pass_count, count);
-- 
2.24.1

[PATCH v3 5/6] kselftest: add fixture variants

[Jakub Kicinski]

Allow users to pass parameters to fixtures.

Each fixture will be evaluated for each of its variants.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v3:
 - separate variant name out with a dot;
 - count variants as "cases" in the opening print.
---
 Documentation/dev-tools/kselftest.rst       |   3 +-
 tools/testing/selftests/kselftest_harness.h | 145 ++++++++++++++++----
 2 files changed, 121 insertions(+), 27 deletions(-)

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index 61ae13c44f91..5d1f56fcd2e7 100644
--- a/Documentation/dev-tools/kselftest.rst
+++ b/Documentation/dev-tools/kselftest.rst
@@ -301,7 +301,8 @@ Helpers
 
 .. kernel-doc:: tools/testing/selftests/kselftest_harness.h
     :functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP
-                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN
+                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN FIXTURE_VARIANT
+                FIXTURE_VARIANT_ADD
 
 Operators
 ---------
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 36ab1b92eb35..1a079afa2d01 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -168,9 +168,15 @@
 
 #define __TEST_IMPL(test_name, _signal) \
 	static void test_name(struct __test_metadata *_metadata); \
+	static inline void wrapper_##test_name( \
+		struct __test_metadata *_metadata, \
+		struct __fixture_variant_metadata *variant) \
+	{ \
+		test_name(_metadata); \
+	} \
 	static struct __test_metadata _##test_name##_object = \
 		{ .name = #test_name, \
-		  .fn = &test_name, \
+		  .fn = &wrapper_##test_name, \
 		  .fixture = &_fixture_global, \
 		  .termsig = _signal, \
 		  .timeout = TEST_TIMEOUT_DEFAULT, }; \
@@ -214,6 +220,7 @@
  * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
  */
 #define FIXTURE(fixture_name) \
+	FIXTURE_VARIANT(fixture_name); \
 	static struct __fixture_metadata _##fixture_name##_fixture_object = \
 		{ .name =  #fixture_name, }; \
 	static void __attribute__((constructor)) \
@@ -245,7 +252,9 @@
 #define FIXTURE_SETUP(fixture_name) \
 	void fixture_name##_setup( \
 		struct __test_metadata __attribute__((unused)) *_metadata, \
-		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
+		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
+		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
+
 /**
  * FIXTURE_TEARDOWN(fixture_name)
  * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
@@ -267,6 +276,58 @@
 		struct __test_metadata __attribute__((unused)) *_metadata, \
 		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
 
+/**
+ * FIXTURE_VARIANT(fixture_name) - Optionally called once per fixture
+ * to declare fixture variant
+ *
+ * @fixture_name: fixture name
+ *
+ * .. code-block:: c
+ *
+ *     FIXTURE_VARIANT(datatype name) {
+ *       type property1;
+ *       ...
+ *     };
+ *
+ * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F()
+ * as *variant*. Variants allow the same tests to be run with different
+ * arguments.
+ */
+#define FIXTURE_VARIANT(fixture_name) struct _fixture_variant_##fixture_name
+
+/**
+ * FIXTURE_VARIANT_ADD(fixture_name, variant_name) - Called once per fixture
+ * variant to setup and register the data
+ *
+ * @fixture_name: fixture name
+ * @variant_name: name of the parameter set
+ *
+ * .. code-block:: c
+ *
+ *     FIXTURE_ADD(datatype name) {
+ *       .property1 = val1;
+ *       ...
+ *     };
+ *
+ * Defines a variant of the test fixture, provided to FIXTURE_SETUP() and
+ * TEST_F() as *variant*. Tests of each fixture will be run once for each
+ * variant.
+ */
+#define FIXTURE_VARIANT_ADD(fixture_name, variant_name) \
+	extern FIXTURE_VARIANT(fixture_name) \
+		_##fixture_name##_##variant_name##_variant; \
+	static struct __fixture_variant_metadata \
+		_##fixture_name##_##variant_name##_object = \
+		{ .name = #variant_name, \
+		  .data = &_##fixture_name##_##variant_name##_variant}; \
+	static void __attribute__((constructor)) \
+		_register_##fixture_name##_##variant_name(void) \
+	{ \
+		__register_fixture_variant(&_##fixture_name##_fixture_object, \
+			&_##fixture_name##_##variant_name##_object);	\
+	} \
+	FIXTURE_VARIANT(fixture_name) _##fixture_name##_##variant_name##_variant =
+
 /**
  * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
  * fixture-based test cases
@@ -297,18 +358,20 @@
 #define __TEST_F_IMPL(fixture_name, test_name, signal, tmout) \
 	static void fixture_name##_##test_name( \
 		struct __test_metadata *_metadata, \
-		FIXTURE_DATA(fixture_name) *self); \
+		FIXTURE_DATA(fixture_name) *self, \
+		const FIXTURE_VARIANT(fixture_name) *variant); \
 	static inline void wrapper_##fixture_name##_##test_name( \
-		struct __test_metadata *_metadata) \
+		struct __test_metadata *_metadata, \
+		struct __fixture_variant_metadata *variant) \
 	{ \
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
-		fixture_name##_setup(_metadata, &self); \
+		fixture_name##_setup(_metadata, &self, variant->data); \
 		/* Let setup failure terminate early. */ \
 		if (!_metadata->passed) \
 			return; \
-		fixture_name##_##test_name(_metadata, &self); \
+		fixture_name##_##test_name(_metadata, &self, variant->data); \
 		fixture_name##_teardown(_metadata, &self); \
 	} \
 	static struct __test_metadata \
@@ -326,7 +389,8 @@
 	} \
 	static void fixture_name##_##test_name( \
 		struct __test_metadata __attribute__((unused)) *_metadata, \
-		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
+		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
+		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
 
 /**
  * TEST_HARNESS_MAIN - Simple wrapper to run the test harness
@@ -661,10 +725,12 @@
 
 /* Contains all the information about a fixture */
 struct __test_metadata;
+struct __fixture_variant_metadata;
 
 struct __fixture_metadata {
 	const char *name;
 	struct __test_metadata *tests;
+	struct __fixture_variant_metadata *variant;
 	struct __fixture_metadata *prev, *next;
 } _fixture_global __attribute__((unused)) = {
 	.name = "global",
@@ -672,7 +738,6 @@ struct __fixture_metadata {
 };
 
 static struct __fixture_metadata *__fixture_list = &_fixture_global;
-static unsigned int __fixture_count;
 static int __constructor_order;
 
 #define _CONSTRUCTOR_ORDER_FORWARD   1
@@ -680,14 +745,27 @@ static int __constructor_order;
 
 static inline void __register_fixture(struct __fixture_metadata *f)
 {
-	__fixture_count++;
 	__LIST_APPEND(__fixture_list, f);
 }
 
+struct __fixture_variant_metadata {
+	const char *name;
+	const void *data;
+	struct __fixture_variant_metadata *prev, *next;
+};
+
+static inline void
+__register_fixture_variant(struct __fixture_metadata *f,
+			  struct __fixture_variant_metadata *variant)
+{
+	__LIST_APPEND(f->variant, variant);
+}
+
 /* Contains all the information for test execution and status checking. */
 struct __test_metadata {
 	const char *name;
-	void (*fn)(struct __test_metadata *);
+	void (*fn)(struct __test_metadata *,
+		   struct __fixture_variant_metadata *);
 	struct __fixture_metadata *fixture;
 	int termsig;
 	int passed;
@@ -698,9 +776,6 @@ struct __test_metadata {
 	struct __test_metadata *prev, *next;
 };
 
-/* Storage for the (global) tests to be run. */
-static unsigned int __test_count;
-
 /*
  * Since constructors are called in reverse order, reverse the test
  * list so tests are run in source declaration order.
@@ -714,7 +789,6 @@ static inline void __register_test(struct __test_metadata *t)
 {
 	struct __fixture_metadata *f = t->fixture;
 
-	__test_count++;
 	__LIST_APPEND(f->tests, t);
 }
 
@@ -729,21 +803,27 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
 }
 
 void __run_test(struct __fixture_metadata *f,
+		struct __fixture_variant_metadata *variant,
 		struct __test_metadata *t)
 {
 	pid_t child_pid;
 	int status;
 
+	/* reset test struct */
 	t->passed = 1;
 	t->trigger = 0;
-	printf("[ RUN      ] %s.%s\n", f->name, t->name);
+	t->step = 0;
+	t->no_print = 0;
+
+	printf("[ RUN      ] %s%s%s.%s\n",
+	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
 	alarm(t->timeout);
 	child_pid = fork();
 	if (child_pid < 0) {
 		printf("ERROR SPAWNING TEST CHILD\n");
 		t->passed = 0;
 	} else if (child_pid == 0) {
-		t->fn(t);
+		t->fn(t, variant);
 		/* return the step that failed or 0 */
 		_exit(t->passed ? 0 : t->step);
 	} else {
@@ -785,31 +865,44 @@ void __run_test(struct __fixture_metadata *f,
 				status);
 		}
 	}
-	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
-	       f->name, t->name);
+	printf("[     %4s ] %s%s%s.%s\n", (t->passed ? "OK" : "FAIL"),
+	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
 	alarm(0);
 }
 
 static int test_harness_run(int __attribute__((unused)) argc,
 			    char __attribute__((unused)) **argv)
 {
+	struct __fixture_variant_metadata no_variant = { .name = "", };
+	struct __fixture_variant_metadata *v;
 	struct __fixture_metadata *f;
 	struct __test_metadata *t;
 	int ret = 0;
+	unsigned int case_count = 0, test_count = 0;
 	unsigned int count = 0;
 	unsigned int pass_count = 0;
 
+	for (f = __fixture_list; f; f = f->next) {
+		for (v = f->variant ?: &no_variant; v; v = v->next) {
+			case_count++;
+			for (t = f->tests; t; t = t->next)
+				test_count++;
+		}
+	}
+
 	/* TODO(wad) add optional arguments similar to gtest. */
 	printf("[==========] Running %u tests from %u test cases.\n",
-	       __test_count, __fixture_count + 1);
+	       test_count, case_count);
 	for (f = __fixture_list; f; f = f->next) {
-		for (t = f->tests; t; t = t->next) {
-			count++;
-			__run_test(f, t);
-			if (t->passed)
-				pass_count++;
-			else
-				ret = 1;
+		for (v = f->variant ?: &no_variant; v; v = v->next) {
+			for (t = f->tests; t; t = t->next) {
+				count++;
+				__run_test(f, v, t);
+				if (t->passed)
+					pass_count++;
+				else
+					ret = 1;
+			}
 		}
 	}
 	printf("[==========] %u / %u tests passed.\n", pass_count, count);
-- 
2.24.1

[PATCH v3 6/6] selftests: tls: run all tests for TLS 1.2 and TLS 1.3

[Jakub Kicinski]

TLS 1.2 and TLS 1.3 differ in the implementation.
Use fixture parameters to run all tests for both
versions, and remove the one-off TLS 1.2 test.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/tls.c | 93 ++++++-------------------------
 1 file changed, 17 insertions(+), 76 deletions(-)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 0ea44d975b6c..c5282e62df75 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -101,6 +101,21 @@ FIXTURE(tls)
 	bool notls;
 };
 
+FIXTURE_VARIANT(tls)
+{
+	unsigned int tls_version;
+};
+
+FIXTURE_VARIANT_ADD(tls, 12)
+{
+	.tls_version = TLS_1_2_VERSION,
+};
+
+FIXTURE_VARIANT_ADD(tls, 13)
+{
+	.tls_version = TLS_1_3_VERSION,
+};
+
 FIXTURE_SETUP(tls)
 {
 	struct tls12_crypto_info_aes_gcm_128 tls12;
@@ -112,7 +127,7 @@ FIXTURE_SETUP(tls)
 	len = sizeof(addr);
 
 	memset(&tls12, 0, sizeof(tls12));
-	tls12.info.version = TLS_1_3_VERSION;
+	tls12.info.version = variant->tls_version;
 	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
 
 	addr.sin_family = AF_INET;
@@ -733,7 +748,7 @@ TEST_F(tls, bidir)
 		struct tls12_crypto_info_aes_gcm_128 tls12;
 
 		memset(&tls12, 0, sizeof(tls12));
-		tls12.info.version = TLS_1_3_VERSION;
+		tls12.info.version = variant->tls_version;
 		tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
 
 		ret = setsockopt(self->fd, SOL_TLS, TLS_RX, &tls12,
@@ -1258,78 +1273,4 @@ TEST(keysizes) {
 	close(cfd);
 }
 
-TEST(tls12) {
-	int fd, cfd;
-	bool notls;
-
-	struct tls12_crypto_info_aes_gcm_128 tls12;
-	struct sockaddr_in addr;
-	socklen_t len;
-	int sfd, ret;
-
-	notls = false;
-	len = sizeof(addr);
-
-	memset(&tls12, 0, sizeof(tls12));
-	tls12.info.version = TLS_1_2_VERSION;
-	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
-
-	addr.sin_family = AF_INET;
-	addr.sin_addr.s_addr = htonl(INADDR_ANY);
-	addr.sin_port = 0;
-
-	fd = socket(AF_INET, SOCK_STREAM, 0);
-	sfd = socket(AF_INET, SOCK_STREAM, 0);
-
-	ret = bind(sfd, &addr, sizeof(addr));
-	ASSERT_EQ(ret, 0);
-	ret = listen(sfd, 10);
-	ASSERT_EQ(ret, 0);
-
-	ret = getsockname(sfd, &addr, &len);
-	ASSERT_EQ(ret, 0);
-
-	ret = connect(fd, &addr, sizeof(addr));
-	ASSERT_EQ(ret, 0);
-
-	ret = setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
-	if (ret != 0) {
-		notls = true;
-		printf("Failure setting TCP_ULP, testing without tls\n");
-	}
-
-	if (!notls) {
-		ret = setsockopt(fd, SOL_TLS, TLS_TX, &tls12,
-				 sizeof(tls12));
-		ASSERT_EQ(ret, 0);
-	}
-
-	cfd = accept(sfd, &addr, &len);
-	ASSERT_GE(cfd, 0);
-
-	if (!notls) {
-		ret = setsockopt(cfd, IPPROTO_TCP, TCP_ULP, "tls",
-				 sizeof("tls"));
-		ASSERT_EQ(ret, 0);
-
-		ret = setsockopt(cfd, SOL_TLS, TLS_RX, &tls12,
-				 sizeof(tls12));
-		ASSERT_EQ(ret, 0);
-	}
-
-	close(sfd);
-
-	char const *test_str = "test_read";
-	int send_len = 10;
-	char buf[10];
-
-	send_len = strlen(test_str) + 1;
-	EXPECT_EQ(send(fd, test_str, send_len, 0), send_len);
-	EXPECT_NE(recv(cfd, buf, send_len, 0), -1);
-	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
-
-	close(fd);
-	close(cfd);
-}
-
 TEST_HARNESS_MAIN
-- 
2.24.1

Re: [PATCH v3 0/6] kselftest: add fixture parameters

[Jakub Kicinski]

On Mon, 16 Mar 2020 15:56:40 -0700 Jakub Kicinski wrote:
> Hi!
> 
> Shuah please consider applying to the kselftest tree.
> 
> This set is an attempt to make running tests for different
> sets of data easier. The direct motivation is the tls
> test which we'd like to run for TLS 1.2 and TLS 1.3,
> but currently there is no easy way to invoke the same
> tests with different parameters.
> 
> Tested all users of kselftest_harness.h.
> 
> v2:
>  - don't run tests by fixture
>  - don't pass params as an explicit argument
> 
> v3:
>  - go back to the orginal implementation with an extra
>    parameter, and running by fixture (Kees);
>  - add LIST_APPEND helper (Kees);
>  - add a dot between fixture and param name (Kees);
>  - rename the params to variants (Tim);
> 
> v1: https://lore.kernel.org/netdev/20200313031752.2332565-1-kuba@kernel.org/
> v2: https://lore.kernel.org/netdev/20200314005501.2446494-1-kuba@kernel.org/

Ugh, sorry I forgot to realign things after the rename :S

I'll send a whitespace-only v4 in a hour, allowing a little bit 
of time in case there are some comments already.

Re: [PATCH v3 1/6] selftests/seccomp: use correct FIXTURE macro

[Kees Cook]

On Mon, Mar 16, 2020 at 03:56:41PM -0700, Jakub Kicinski wrote:
> Quoting kdoc:
> 
> FIXTURE_DATA:
>  * This call may be used when the type of the fixture data
>  * is needed.  In general, this should not be needed unless
>  * the *self* is being passed to a helper directly.
> 
> FIXTURE:
>  * Defines the data provided to TEST_F()-defined tests as *self*.  It should be
>  * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
> 
> seccomp should use FIXTURE to declare types.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

-Kees

> ---
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
> index ee1b727ede04..7bf82fb07f67 100644
> --- a/tools/testing/selftests/seccomp/seccomp_bpf.c
> +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
> @@ -909,7 +909,7 @@ TEST(ERRNO_order)
>  	EXPECT_EQ(12, errno);
>  }
>  
> -FIXTURE_DATA(TRAP) {
> +FIXTURE(TRAP) {
>  	struct sock_fprog prog;
>  };
>  
> @@ -1020,7 +1020,7 @@ TEST_F(TRAP, handler)
>  	EXPECT_NE(0, (unsigned long)sigsys->_call_addr);
>  }
>  
> -FIXTURE_DATA(precedence) {
> +FIXTURE(precedence) {
>  	struct sock_fprog allow;
>  	struct sock_fprog log;
>  	struct sock_fprog trace;
> @@ -1509,7 +1509,7 @@ void tracer_poke(struct __test_metadata *_metadata, pid_t tracee, int status,
>  	EXPECT_EQ(0, ret);
>  }
>  
> -FIXTURE_DATA(TRACE_poke) {
> +FIXTURE(TRACE_poke) {
>  	struct sock_fprog prog;
>  	pid_t tracer;
>  	long poked;
> @@ -1817,7 +1817,7 @@ void tracer_ptrace(struct __test_metadata *_metadata, pid_t tracee,
>  		change_syscall(_metadata, tracee, -1, -ESRCH);
>  }
>  
> -FIXTURE_DATA(TRACE_syscall) {
> +FIXTURE(TRACE_syscall) {
>  	struct sock_fprog prog;
>  	pid_t tracer, mytid, mypid, parent;
>  };
> @@ -2321,7 +2321,7 @@ struct tsync_sibling {
>  		}							\
>  	} while (0)
>  
> -FIXTURE_DATA(TSYNC) {
> +FIXTURE(TSYNC) {
>  	struct sock_fprog root_prog, apply_prog;
>  	struct tsync_sibling sibling[TSYNC_SIBLINGS];
>  	sem_t started;
> -- 
> 2.24.1
> 

-- 
Kees Cook

Re: [PATCH v3 2/6] kselftest: factor out list manipulation to a helper

[Kees Cook]

On Mon, Mar 16, 2020 at 03:56:42PM -0700, Jakub Kicinski wrote:
> Kees suggest to factor out the list append code to a macro,
> since following commits need it, which leads to code duplication.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

-Kees

> ---
>  tools/testing/selftests/kselftest_harness.h | 42 ++++++++++++---------
>  1 file changed, 24 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 5336b26506ab..aaf58fffc8f7 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -631,6 +631,29 @@
>  	} \
>  } while (0); OPTIONAL_HANDLER(_assert)
>  
> +/* List helpers */
> +#define __LIST_APPEND(head, item) \
> +{ \
> +	/* Circular linked list where only prev is circular. */ \
> +	if (head == NULL) { \
> +		head = item; \
> +		item->next = NULL; \
> +		item->prev = item; \
> +		return;	\
> +	} \
> +	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) { \
> +		item->next = NULL; \
> +		item->prev = head->prev; \
> +		item->prev->next = item; \
> +		head->prev = item; \
> +	} else { \
> +		item->next = head; \
> +		item->next->prev = item; \
> +		item->prev = item; \
> +		head = item; \
> +	} \
> +}
> +
>  /* Contains all the information for test execution and status checking. */
>  struct __test_metadata {
>  	const char *name;
> @@ -665,24 +688,7 @@ static int __constructor_order;
>  static inline void __register_test(struct __test_metadata *t)
>  {
>  	__test_count++;
> -	/* Circular linked list where only prev is circular. */
> -	if (__test_list == NULL) {
> -		__test_list = t;
> -		t->next = NULL;
> -		t->prev = t;
> -		return;
> -	}
> -	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
> -		t->next = NULL;
> -		t->prev = __test_list->prev;
> -		t->prev->next = t;
> -		__test_list->prev = t;
> -	} else {
> -		t->next = __test_list;
> -		t->next->prev = t;
> -		t->prev = t;
> -		__test_list = t;
> -	}
> +	__LIST_APPEND(__test_list, t);
>  }
>  
>  static inline int __bail(int for_realz, bool no_print, __u8 step)
> -- 
> 2.24.1
> 

-- 
Kees Cook

Re: [PATCH v3 3/6] kselftest: create fixture objects

[Kees Cook]

On Mon, Mar 16, 2020 at 03:56:43PM -0700, Jakub Kicinski wrote:
> Grouping tests by fixture will allow us to parametrize
> test runs. Create full objects for fixtures.
> 
> Add a "global" fixture for tests without a fixture.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

-Kees

> ---
>  tools/testing/selftests/kselftest_harness.h | 46 ++++++++++++++++-----
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index aaf58fffc8f7..0f68943d6f04 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -169,8 +169,10 @@
>  #define __TEST_IMPL(test_name, _signal) \
>  	static void test_name(struct __test_metadata *_metadata); \
>  	static struct __test_metadata _##test_name##_object = \
> -		{ .name = "global." #test_name, \
> -		  .fn = &test_name, .termsig = _signal, \
> +		{ .name = #test_name, \
> +		  .fn = &test_name, \
> +		  .fixture = &_fixture_global, \
> +		  .termsig = _signal, \
>  		  .timeout = TEST_TIMEOUT_DEFAULT, }; \
>  	static void __attribute__((constructor)) _register_##test_name(void) \
>  	{ \
> @@ -212,10 +214,12 @@
>   * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
>   */
>  #define FIXTURE(fixture_name) \
> +	static struct __fixture_metadata _##fixture_name##_fixture_object = \
> +		{ .name =  #fixture_name, }; \
>  	static void __attribute__((constructor)) \
>  	_register_##fixture_name##_data(void) \
>  	{ \
> -		__fixture_count++; \
> +		__register_fixture(&_##fixture_name##_fixture_object); \
>  	} \
>  	FIXTURE_DATA(fixture_name)
>  
> @@ -309,8 +313,9 @@
>  	} \
>  	static struct __test_metadata \
>  		      _##fixture_name##_##test_name##_object = { \
> -		.name = #fixture_name "." #test_name, \
> +		.name = #test_name, \
>  		.fn = &wrapper_##fixture_name##_##test_name, \
> +		.fixture = &_##fixture_name##_fixture_object, \
>  		.termsig = signal, \
>  		.timeout = tmout, \
>  	 }; \
> @@ -654,10 +659,33 @@
>  	} \
>  }
>  
> +/* Contains all the information about a fixture */
> +struct __fixture_metadata {
> +	const char *name;
> +	struct __fixture_metadata *prev, *next;
> +} _fixture_global __attribute__((unused)) = {
> +	.name = "global",
> +	.prev = &_fixture_global,
> +};
> +
> +static struct __fixture_metadata *__fixture_list = &_fixture_global;
> +static unsigned int __fixture_count;
> +static int __constructor_order;
> +
> +#define _CONSTRUCTOR_ORDER_FORWARD   1
> +#define _CONSTRUCTOR_ORDER_BACKWARD -1
> +
> +static inline void __register_fixture(struct __fixture_metadata *f)
> +{
> +	__fixture_count++;
> +	__LIST_APPEND(__fixture_list, f);
> +}
> +
>  /* Contains all the information for test execution and status checking. */
>  struct __test_metadata {
>  	const char *name;
>  	void (*fn)(struct __test_metadata *);
> +	struct __fixture_metadata *fixture;
>  	int termsig;
>  	int passed;
>  	int trigger; /* extra handler after the evaluation */
> @@ -670,11 +698,6 @@ struct __test_metadata {
>  /* Storage for the (global) tests to be run. */
>  static struct __test_metadata *__test_list;
>  static unsigned int __test_count;
> -static unsigned int __fixture_count;
> -static int __constructor_order;
> -
> -#define _CONSTRUCTOR_ORDER_FORWARD   1
> -#define _CONSTRUCTOR_ORDER_BACKWARD -1
>  
>  /*
>   * Since constructors are called in reverse order, reverse the test
> @@ -708,7 +731,7 @@ void __run_test(struct __test_metadata *t)
>  
>  	t->passed = 1;
>  	t->trigger = 0;
> -	printf("[ RUN      ] %s\n", t->name);
> +	printf("[ RUN      ] %s.%s\n", t->fixture->name, t->name);
>  	alarm(t->timeout);
>  	child_pid = fork();
>  	if (child_pid < 0) {
> @@ -757,7 +780,8 @@ void __run_test(struct __test_metadata *t)
>  				status);
>  		}
>  	}
> -	printf("[     %4s ] %s\n", (t->passed ? "OK" : "FAIL"), t->name);
> +	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
> +	       t->fixture->name, t->name);
>  	alarm(0);
>  }
>  
> -- 
> 2.24.1
> 

-- 
Kees Cook

Re: [PATCH v3 4/6] kselftest: run tests by fixture

[Kees Cook]

On Mon, Mar 16, 2020 at 03:56:44PM -0700, Jakub Kicinski wrote:
> Now that all tests have a fixture object move from a global
> list of tests to a list of tests per fixture.
> 
> Order of tests may change as we will now group and run test
> fixture by fixture, rather than in declaration order.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/kselftest_harness.h | 32 +++++++++++++--------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 0f68943d6f04..36ab1b92eb35 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -660,8 +660,11 @@
>  }
>  
>  /* Contains all the information about a fixture */
> +struct __test_metadata;
> +

Comment should be moved under this (it applies to __fixture_metadata not
__test_metadata).

>  struct __fixture_metadata {
>  	const char *name;
> +	struct __test_metadata *tests;
>  	struct __fixture_metadata *prev, *next;
>  } _fixture_global __attribute__((unused)) = {
>  	.name = "global",
> @@ -696,7 +699,6 @@ struct __test_metadata {
>  };
>  
>  /* Storage for the (global) tests to be run. */
> -static struct __test_metadata *__test_list;
>  static unsigned int __test_count;
>  
>  /*
> @@ -710,8 +712,10 @@ static unsigned int __test_count;
>   */
>  static inline void __register_test(struct __test_metadata *t)
>  {
> +	struct __fixture_metadata *f = t->fixture;
> +
>  	__test_count++;
> -	__LIST_APPEND(__test_list, t);
> +	__LIST_APPEND(f->tests, t);

Not a big deal, but why not just "f->fixture->tests" here instead of a
separate variable?

>  }
>  
>  static inline int __bail(int for_realz, bool no_print, __u8 step)
> @@ -724,14 +728,15 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
>  	return 0;
>  }
>  
> -void __run_test(struct __test_metadata *t)
> +void __run_test(struct __fixture_metadata *f,
> +		struct __test_metadata *t)
>  {
>  	pid_t child_pid;
>  	int status;
>  
>  	t->passed = 1;
>  	t->trigger = 0;
> -	printf("[ RUN      ] %s.%s\n", t->fixture->name, t->name);
> +	printf("[ RUN      ] %s.%s\n", f->name, t->name);
>  	alarm(t->timeout);
>  	child_pid = fork();
>  	if (child_pid < 0) {
> @@ -781,13 +786,14 @@ void __run_test(struct __test_metadata *t)
>  		}
>  	}
>  	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
> -	       t->fixture->name, t->name);
> +	       f->name, t->name);
>  	alarm(0);
>  }
>  
>  static int test_harness_run(int __attribute__((unused)) argc,
>  			    char __attribute__((unused)) **argv)
>  {
> +	struct __fixture_metadata *f;
>  	struct __test_metadata *t;
>  	int ret = 0;
>  	unsigned int count = 0;
> @@ -796,13 +802,15 @@ static int test_harness_run(int __attribute__((unused)) argc,
>  	/* TODO(wad) add optional arguments similar to gtest. */
>  	printf("[==========] Running %u tests from %u test cases.\n",
>  	       __test_count, __fixture_count + 1);
> -	for (t = __test_list; t; t = t->next) {
> -		count++;
> -		__run_test(t);
> -		if (t->passed)
> -			pass_count++;
> -		else
> -			ret = 1;
> +	for (f = __fixture_list; f; f = f->next) {
> +		for (t = f->tests; t; t = t->next) {
> +			count++;
> +			__run_test(f, t);
> +			if (t->passed)
> +				pass_count++;
> +			else
> +				ret = 1;
> +		}
>  	}
>  	printf("[==========] %u / %u tests passed.\n", pass_count, count);
>  	printf("[  %s  ]\n", (ret ? "FAILED" : "PASSED"));
> -- 
> 2.24.1
> 

But, with at least the first comment moved:

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

-- 
Kees Cook

Re: [PATCH v3 5/6] kselftest: add fixture parameters

[Kees Cook]

On Mon, Mar 16, 2020 at 03:56:45PM -0700, Jakub Kicinski wrote:
> Allow users to pass parameters to fixtures.
> 
> Each fixture will be evaluated for each of its parameter
> sets.

This commit log (and subject) needs to be reworded/expanded slightly.
Perhaps:


Subject: kselftest: add fixture variants

Allow users to build parameterized variants of fixtures.

If fixtures want variants, they call FIXTURE_VARIANT() to declare the
structure to fill for each variant. Each fixture will be re-run for each
of the variants defined by calling FIXTURE_VARIANT_ADD() with the
differing parameters initializing the structure.

Since tests are being re-run, additional initialization (steps,
no_print) is also added.




> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> v3:
>  - separate variant name out with a dot.
> ---
>  Documentation/dev-tools/kselftest.rst       |   3 +-
>  tools/testing/selftests/kselftest_harness.h | 145 ++++++++++++++++----
>  2 files changed, 121 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
> index 61ae13c44f91..5d1f56fcd2e7 100644
> --- a/Documentation/dev-tools/kselftest.rst
> +++ b/Documentation/dev-tools/kselftest.rst
> @@ -301,7 +301,8 @@ Helpers
>  
>  .. kernel-doc:: tools/testing/selftests/kselftest_harness.h
>      :functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP
> -                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN
> +                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN FIXTURE_VARIANT
> +                FIXTURE_VARIANT_ADD
>  
>  Operators
>  ---------
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 36ab1b92eb35..1a079afa2d01 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -168,9 +168,15 @@
>  
>  #define __TEST_IMPL(test_name, _signal) \
>  	static void test_name(struct __test_metadata *_metadata); \
> +	static inline void wrapper_##test_name( \
> +		struct __test_metadata *_metadata, \
> +		struct __fixture_variant_metadata *variant) \
> +	{ \
> +		test_name(_metadata); \
> +	} \
>  	static struct __test_metadata _##test_name##_object = \
>  		{ .name = #test_name, \
> -		  .fn = &test_name, \
> +		  .fn = &wrapper_##test_name, \
>  		  .fixture = &_fixture_global, \
>  		  .termsig = _signal, \
>  		  .timeout = TEST_TIMEOUT_DEFAULT, }; \
> @@ -214,6 +220,7 @@
>   * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
>   */
>  #define FIXTURE(fixture_name) \
> +	FIXTURE_VARIANT(fixture_name); \
>  	static struct __fixture_metadata _##fixture_name##_fixture_object = \
>  		{ .name =  #fixture_name, }; \
>  	static void __attribute__((constructor)) \
> @@ -245,7 +252,9 @@
>  #define FIXTURE_SETUP(fixture_name) \
>  	void fixture_name##_setup( \
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
> -		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
> +		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
> +		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
> +
>  /**
>   * FIXTURE_TEARDOWN(fixture_name)
>   * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
> @@ -267,6 +276,58 @@
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
>  		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
>  
> +/**
> + * FIXTURE_VARIANT(fixture_name) - Optionally called once per fixture
> + * to declare fixture variant
> + *
> + * @fixture_name: fixture name
> + *
> + * .. code-block:: c
> + *
> + *     FIXTURE_VARIANT(datatype name) {
> + *       type property1;
> + *       ...
> + *     };
> + *
> + * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F()
> + * as *variant*. Variants allow the same tests to be run with different
> + * arguments.
> + */
> +#define FIXTURE_VARIANT(fixture_name) struct _fixture_variant_##fixture_name
> +
> +/**
> + * FIXTURE_VARIANT_ADD(fixture_name, variant_name) - Called once per fixture
> + * variant to setup and register the data
> + *
> + * @fixture_name: fixture name
> + * @variant_name: name of the parameter set
> + *
> + * .. code-block:: c
> + *
> + *     FIXTURE_ADD(datatype name) {
> + *       .property1 = val1;
> + *       ...
> + *     };
> + *
> + * Defines a variant of the test fixture, provided to FIXTURE_SETUP() and
> + * TEST_F() as *variant*. Tests of each fixture will be run once for each
> + * variant.
> + */
> +#define FIXTURE_VARIANT_ADD(fixture_name, variant_name) \
> +	extern FIXTURE_VARIANT(fixture_name) \
> +		_##fixture_name##_##variant_name##_variant; \
> +	static struct __fixture_variant_metadata \
> +		_##fixture_name##_##variant_name##_object = \
> +		{ .name = #variant_name, \
> +		  .data = &_##fixture_name##_##variant_name##_variant}; \
> +	static void __attribute__((constructor)) \
> +		_register_##fixture_name##_##variant_name(void) \
> +	{ \
> +		__register_fixture_variant(&_##fixture_name##_fixture_object, \
> +			&_##fixture_name##_##variant_name##_object);	\
> +	} \
> +	FIXTURE_VARIANT(fixture_name) _##fixture_name##_##variant_name##_variant =
> +
>  /**
>   * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
>   * fixture-based test cases
> @@ -297,18 +358,20 @@
>  #define __TEST_F_IMPL(fixture_name, test_name, signal, tmout) \
>  	static void fixture_name##_##test_name( \
>  		struct __test_metadata *_metadata, \
> -		FIXTURE_DATA(fixture_name) *self); \
> +		FIXTURE_DATA(fixture_name) *self, \
> +		const FIXTURE_VARIANT(fixture_name) *variant); \
>  	static inline void wrapper_##fixture_name##_##test_name( \
> -		struct __test_metadata *_metadata) \
> +		struct __test_metadata *_metadata, \
> +		struct __fixture_variant_metadata *variant) \
>  	{ \
>  		/* fixture data is alloced, setup, and torn down per call. */ \
>  		FIXTURE_DATA(fixture_name) self; \
>  		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
> -		fixture_name##_setup(_metadata, &self); \
> +		fixture_name##_setup(_metadata, &self, variant->data); \
>  		/* Let setup failure terminate early. */ \
>  		if (!_metadata->passed) \
>  			return; \
> -		fixture_name##_##test_name(_metadata, &self); \
> +		fixture_name##_##test_name(_metadata, &self, variant->data); \
>  		fixture_name##_teardown(_metadata, &self); \
>  	} \
>  	static struct __test_metadata \
> @@ -326,7 +389,8 @@
>  	} \
>  	static void fixture_name##_##test_name( \
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
> -		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
> +		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
> +		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
>  
>  /**
>   * TEST_HARNESS_MAIN - Simple wrapper to run the test harness
> @@ -661,10 +725,12 @@
>  
>  /* Contains all the information about a fixture */
>  struct __test_metadata;
> +struct __fixture_variant_metadata;
>  
>  struct __fixture_metadata {
>  	const char *name;
>  	struct __test_metadata *tests;
> +	struct __fixture_variant_metadata *variant;
>  	struct __fixture_metadata *prev, *next;
>  } _fixture_global __attribute__((unused)) = {
>  	.name = "global",
> @@ -672,7 +738,6 @@ struct __fixture_metadata {
>  };
>  
>  static struct __fixture_metadata *__fixture_list = &_fixture_global;
> -static unsigned int __fixture_count;
>  static int __constructor_order;
>  
>  #define _CONSTRUCTOR_ORDER_FORWARD   1
> @@ -680,14 +745,27 @@ static int __constructor_order;
>  
>  static inline void __register_fixture(struct __fixture_metadata *f)
>  {
> -	__fixture_count++;
>  	__LIST_APPEND(__fixture_list, f);
>  }
>  
> +struct __fixture_variant_metadata {
> +	const char *name;
> +	const void *data;
> +	struct __fixture_variant_metadata *prev, *next;
> +};
> +
> +static inline void
> +__register_fixture_variant(struct __fixture_metadata *f,
> +			  struct __fixture_variant_metadata *variant)
> +{
> +	__LIST_APPEND(f->variant, variant);
> +}
> +
>  /* Contains all the information for test execution and status checking. */
>  struct __test_metadata {
>  	const char *name;
> -	void (*fn)(struct __test_metadata *);
> +	void (*fn)(struct __test_metadata *,
> +		   struct __fixture_variant_metadata *);
>  	struct __fixture_metadata *fixture;
>  	int termsig;
>  	int passed;
> @@ -698,9 +776,6 @@ struct __test_metadata {
>  	struct __test_metadata *prev, *next;
>  };
>  
> -/* Storage for the (global) tests to be run. */
> -static unsigned int __test_count;
> -
>  /*
>   * Since constructors are called in reverse order, reverse the test
>   * list so tests are run in source declaration order.
> @@ -714,7 +789,6 @@ static inline void __register_test(struct __test_metadata *t)
>  {
>  	struct __fixture_metadata *f = t->fixture;
>  
> -	__test_count++;
>  	__LIST_APPEND(f->tests, t);
>  }
>  
> @@ -729,21 +803,27 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
>  }
>  
>  void __run_test(struct __fixture_metadata *f,
> +		struct __fixture_variant_metadata *variant,
>  		struct __test_metadata *t)
>  {
>  	pid_t child_pid;
>  	int status;
>  
> +	/* reset test struct */
>  	t->passed = 1;
>  	t->trigger = 0;
> -	printf("[ RUN      ] %s.%s\n", f->name, t->name);
> +	t->step = 0;
> +	t->no_print = 0;

I called this out in the commit log. This will need some merge attention
when my series for timeouts is merged too (since the new "timed_out"
will need to be initialized here too), but you don't have to worry about
that yet.

> +
> +	printf("[ RUN      ] %s%s%s.%s\n",
> +	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
>  	alarm(t->timeout);
>  	child_pid = fork();
>  	if (child_pid < 0) {
>  		printf("ERROR SPAWNING TEST CHILD\n");
>  		t->passed = 0;
>  	} else if (child_pid == 0) {
> -		t->fn(t);
> +		t->fn(t, variant);
>  		/* return the step that failed or 0 */
>  		_exit(t->passed ? 0 : t->step);
>  	} else {
> @@ -785,31 +865,44 @@ void __run_test(struct __fixture_metadata *f,
>  				status);
>  		}
>  	}
> -	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
> -	       f->name, t->name);
> +	printf("[     %4s ] %s%s%s.%s\n", (t->passed ? "OK" : "FAIL"),
> +	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
>  	alarm(0);
>  }
>  
>  static int test_harness_run(int __attribute__((unused)) argc,
>  			    char __attribute__((unused)) **argv)
>  {
> +	struct __fixture_variant_metadata no_variant = { .name = "", };
> +	struct __fixture_variant_metadata *v;
>  	struct __fixture_metadata *f;
>  	struct __test_metadata *t;
>  	int ret = 0;
> +	unsigned int case_count = 0, test_count = 0;
>  	unsigned int count = 0;
>  	unsigned int pass_count = 0;
>  
> +	for (f = __fixture_list; f; f = f->next) {
> +		for (v = f->variant ?: &no_variant; v; v = v->next) {
> +			case_count++;
> +			for (t = f->tests; t; t = t->next)
> +				test_count++;
> +		}
> +	}
> +
>  	/* TODO(wad) add optional arguments similar to gtest. */
>  	printf("[==========] Running %u tests from %u test cases.\n",
> -	       __test_count, __fixture_count + 1);
> +	       test_count, case_count);
>  	for (f = __fixture_list; f; f = f->next) {
> -		for (t = f->tests; t; t = t->next) {
> -			count++;
> -			__run_test(f, t);
> -			if (t->passed)
> -				pass_count++;
> -			else
> -				ret = 1;
> +		for (v = f->variant ?: &no_variant; v; v = v->next) {
> +			for (t = f->tests; t; t = t->next) {
> +				count++;
> +				__run_test(f, v, t);
> +				if (t->passed)
> +					pass_count++;
> +				else
> +					ret = 1;
> +			}
>  		}
>  	}
>  	printf("[==========] %u / %u tests passed.\n", pass_count, count);
> -- 
> 2.24.1
> 

Otherwise, with those things fixed, yes:

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

-- 
Kees Cook

Re: [PATCH v3 5/6] kselftest: add fixture variants

[Kees Cook]

v3.1 ;) My suggestions stand for the expansion of this commit log,
though. :)

-Kees

On Mon, Mar 16, 2020 at 03:56:46PM -0700, Jakub Kicinski wrote:
> Allow users to pass parameters to fixtures.
> 
> Each fixture will be evaluated for each of its variants.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> --
> v3:
>  - separate variant name out with a dot;
>  - count variants as "cases" in the opening print.
> ---
>  Documentation/dev-tools/kselftest.rst       |   3 +-
>  tools/testing/selftests/kselftest_harness.h | 145 ++++++++++++++++----
>  2 files changed, 121 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
> index 61ae13c44f91..5d1f56fcd2e7 100644
> --- a/Documentation/dev-tools/kselftest.rst
> +++ b/Documentation/dev-tools/kselftest.rst
> @@ -301,7 +301,8 @@ Helpers
>  
>  .. kernel-doc:: tools/testing/selftests/kselftest_harness.h
>      :functions: TH_LOG TEST TEST_SIGNAL FIXTURE FIXTURE_DATA FIXTURE_SETUP
> -                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN
> +                FIXTURE_TEARDOWN TEST_F TEST_HARNESS_MAIN FIXTURE_VARIANT
> +                FIXTURE_VARIANT_ADD
>  
>  Operators
>  ---------
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 36ab1b92eb35..1a079afa2d01 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -168,9 +168,15 @@
>  
>  #define __TEST_IMPL(test_name, _signal) \
>  	static void test_name(struct __test_metadata *_metadata); \
> +	static inline void wrapper_##test_name( \
> +		struct __test_metadata *_metadata, \
> +		struct __fixture_variant_metadata *variant) \
> +	{ \
> +		test_name(_metadata); \
> +	} \
>  	static struct __test_metadata _##test_name##_object = \
>  		{ .name = #test_name, \
> -		  .fn = &test_name, \
> +		  .fn = &wrapper_##test_name, \
>  		  .fixture = &_fixture_global, \
>  		  .termsig = _signal, \
>  		  .timeout = TEST_TIMEOUT_DEFAULT, }; \
> @@ -214,6 +220,7 @@
>   * populated and cleaned up using FIXTURE_SETUP() and FIXTURE_TEARDOWN().
>   */
>  #define FIXTURE(fixture_name) \
> +	FIXTURE_VARIANT(fixture_name); \
>  	static struct __fixture_metadata _##fixture_name##_fixture_object = \
>  		{ .name =  #fixture_name, }; \
>  	static void __attribute__((constructor)) \
> @@ -245,7 +252,9 @@
>  #define FIXTURE_SETUP(fixture_name) \
>  	void fixture_name##_setup( \
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
> -		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
> +		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
> +		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
> +
>  /**
>   * FIXTURE_TEARDOWN(fixture_name)
>   * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
> @@ -267,6 +276,58 @@
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
>  		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
>  
> +/**
> + * FIXTURE_VARIANT(fixture_name) - Optionally called once per fixture
> + * to declare fixture variant
> + *
> + * @fixture_name: fixture name
> + *
> + * .. code-block:: c
> + *
> + *     FIXTURE_VARIANT(datatype name) {
> + *       type property1;
> + *       ...
> + *     };
> + *
> + * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F()
> + * as *variant*. Variants allow the same tests to be run with different
> + * arguments.
> + */
> +#define FIXTURE_VARIANT(fixture_name) struct _fixture_variant_##fixture_name
> +
> +/**
> + * FIXTURE_VARIANT_ADD(fixture_name, variant_name) - Called once per fixture
> + * variant to setup and register the data
> + *
> + * @fixture_name: fixture name
> + * @variant_name: name of the parameter set
> + *
> + * .. code-block:: c
> + *
> + *     FIXTURE_ADD(datatype name) {
> + *       .property1 = val1;
> + *       ...
> + *     };
> + *
> + * Defines a variant of the test fixture, provided to FIXTURE_SETUP() and
> + * TEST_F() as *variant*. Tests of each fixture will be run once for each
> + * variant.
> + */
> +#define FIXTURE_VARIANT_ADD(fixture_name, variant_name) \
> +	extern FIXTURE_VARIANT(fixture_name) \
> +		_##fixture_name##_##variant_name##_variant; \
> +	static struct __fixture_variant_metadata \
> +		_##fixture_name##_##variant_name##_object = \
> +		{ .name = #variant_name, \
> +		  .data = &_##fixture_name##_##variant_name##_variant}; \
> +	static void __attribute__((constructor)) \
> +		_register_##fixture_name##_##variant_name(void) \
> +	{ \
> +		__register_fixture_variant(&_##fixture_name##_fixture_object, \
> +			&_##fixture_name##_##variant_name##_object);	\
> +	} \
> +	FIXTURE_VARIANT(fixture_name) _##fixture_name##_##variant_name##_variant =
> +
>  /**
>   * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
>   * fixture-based test cases
> @@ -297,18 +358,20 @@
>  #define __TEST_F_IMPL(fixture_name, test_name, signal, tmout) \
>  	static void fixture_name##_##test_name( \
>  		struct __test_metadata *_metadata, \
> -		FIXTURE_DATA(fixture_name) *self); \
> +		FIXTURE_DATA(fixture_name) *self, \
> +		const FIXTURE_VARIANT(fixture_name) *variant); \
>  	static inline void wrapper_##fixture_name##_##test_name( \
> -		struct __test_metadata *_metadata) \
> +		struct __test_metadata *_metadata, \
> +		struct __fixture_variant_metadata *variant) \
>  	{ \
>  		/* fixture data is alloced, setup, and torn down per call. */ \
>  		FIXTURE_DATA(fixture_name) self; \
>  		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
> -		fixture_name##_setup(_metadata, &self); \
> +		fixture_name##_setup(_metadata, &self, variant->data); \
>  		/* Let setup failure terminate early. */ \
>  		if (!_metadata->passed) \
>  			return; \
> -		fixture_name##_##test_name(_metadata, &self); \
> +		fixture_name##_##test_name(_metadata, &self, variant->data); \
>  		fixture_name##_teardown(_metadata, &self); \
>  	} \
>  	static struct __test_metadata \
> @@ -326,7 +389,8 @@
>  	} \
>  	static void fixture_name##_##test_name( \
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
> -		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
> +		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self, \
> +		const FIXTURE_VARIANT(fixture_name) __attribute__((unused)) *variant)
>  
>  /**
>   * TEST_HARNESS_MAIN - Simple wrapper to run the test harness
> @@ -661,10 +725,12 @@
>  
>  /* Contains all the information about a fixture */
>  struct __test_metadata;
> +struct __fixture_variant_metadata;
>  
>  struct __fixture_metadata {
>  	const char *name;
>  	struct __test_metadata *tests;
> +	struct __fixture_variant_metadata *variant;
>  	struct __fixture_metadata *prev, *next;
>  } _fixture_global __attribute__((unused)) = {
>  	.name = "global",
> @@ -672,7 +738,6 @@ struct __fixture_metadata {
>  };
>  
>  static struct __fixture_metadata *__fixture_list = &_fixture_global;
> -static unsigned int __fixture_count;
>  static int __constructor_order;
>  
>  #define _CONSTRUCTOR_ORDER_FORWARD   1
> @@ -680,14 +745,27 @@ static int __constructor_order;
>  
>  static inline void __register_fixture(struct __fixture_metadata *f)
>  {
> -	__fixture_count++;
>  	__LIST_APPEND(__fixture_list, f);
>  }
>  
> +struct __fixture_variant_metadata {
> +	const char *name;
> +	const void *data;
> +	struct __fixture_variant_metadata *prev, *next;
> +};
> +
> +static inline void
> +__register_fixture_variant(struct __fixture_metadata *f,
> +			  struct __fixture_variant_metadata *variant)
> +{
> +	__LIST_APPEND(f->variant, variant);
> +}
> +
>  /* Contains all the information for test execution and status checking. */
>  struct __test_metadata {
>  	const char *name;
> -	void (*fn)(struct __test_metadata *);
> +	void (*fn)(struct __test_metadata *,
> +		   struct __fixture_variant_metadata *);
>  	struct __fixture_metadata *fixture;
>  	int termsig;
>  	int passed;
> @@ -698,9 +776,6 @@ struct __test_metadata {
>  	struct __test_metadata *prev, *next;
>  };
>  
> -/* Storage for the (global) tests to be run. */
> -static unsigned int __test_count;
> -
>  /*
>   * Since constructors are called in reverse order, reverse the test
>   * list so tests are run in source declaration order.
> @@ -714,7 +789,6 @@ static inline void __register_test(struct __test_metadata *t)
>  {
>  	struct __fixture_metadata *f = t->fixture;
>  
> -	__test_count++;
>  	__LIST_APPEND(f->tests, t);
>  }
>  
> @@ -729,21 +803,27 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
>  }
>  
>  void __run_test(struct __fixture_metadata *f,
> +		struct __fixture_variant_metadata *variant,
>  		struct __test_metadata *t)
>  {
>  	pid_t child_pid;
>  	int status;
>  
> +	/* reset test struct */
>  	t->passed = 1;
>  	t->trigger = 0;
> -	printf("[ RUN      ] %s.%s\n", f->name, t->name);
> +	t->step = 0;
> +	t->no_print = 0;
> +
> +	printf("[ RUN      ] %s%s%s.%s\n",
> +	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
>  	alarm(t->timeout);
>  	child_pid = fork();
>  	if (child_pid < 0) {
>  		printf("ERROR SPAWNING TEST CHILD\n");
>  		t->passed = 0;
>  	} else if (child_pid == 0) {
> -		t->fn(t);
> +		t->fn(t, variant);
>  		/* return the step that failed or 0 */
>  		_exit(t->passed ? 0 : t->step);
>  	} else {
> @@ -785,31 +865,44 @@ void __run_test(struct __fixture_metadata *f,
>  				status);
>  		}
>  	}
> -	printf("[     %4s ] %s.%s\n", (t->passed ? "OK" : "FAIL"),
> -	       f->name, t->name);
> +	printf("[     %4s ] %s%s%s.%s\n", (t->passed ? "OK" : "FAIL"),
> +	       f->name, variant->name[0] ? "." : "", variant->name, t->name);
>  	alarm(0);
>  }
>  
>  static int test_harness_run(int __attribute__((unused)) argc,
>  			    char __attribute__((unused)) **argv)
>  {
> +	struct __fixture_variant_metadata no_variant = { .name = "", };
> +	struct __fixture_variant_metadata *v;
>  	struct __fixture_metadata *f;
>  	struct __test_metadata *t;
>  	int ret = 0;
> +	unsigned int case_count = 0, test_count = 0;
>  	unsigned int count = 0;
>  	unsigned int pass_count = 0;
>  
> +	for (f = __fixture_list; f; f = f->next) {
> +		for (v = f->variant ?: &no_variant; v; v = v->next) {
> +			case_count++;
> +			for (t = f->tests; t; t = t->next)
> +				test_count++;
> +		}
> +	}
> +
>  	/* TODO(wad) add optional arguments similar to gtest. */
>  	printf("[==========] Running %u tests from %u test cases.\n",
> -	       __test_count, __fixture_count + 1);
> +	       test_count, case_count);
>  	for (f = __fixture_list; f; f = f->next) {
> -		for (t = f->tests; t; t = t->next) {
> -			count++;
> -			__run_test(f, t);
> -			if (t->passed)
> -				pass_count++;
> -			else
> -				ret = 1;
> +		for (v = f->variant ?: &no_variant; v; v = v->next) {
> +			for (t = f->tests; t; t = t->next) {
> +				count++;
> +				__run_test(f, v, t);
> +				if (t->passed)
> +					pass_count++;
> +				else
> +					ret = 1;
> +			}
>  		}
>  	}
>  	printf("[==========] %u / %u tests passed.\n", pass_count, count);
> -- 
> 2.24.1
> 

-- 
Kees Cook

Re: [PATCH v3 6/6] selftests: tls: run all tests for TLS 1.2 and TLS 1.3

[Kees Cook]

On Mon, Mar 16, 2020 at 03:56:47PM -0700, Jakub Kicinski wrote:
> TLS 1.2 and TLS 1.3 differ in the implementation.
> Use fixture parameters to run all tests for both
> versions, and remove the one-off TLS 1.2 test.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/net/tls.c | 93 ++++++-------------------------
>  1 file changed, 17 insertions(+), 76 deletions(-)

The diffstat alone justifies the variants feature! :) Thanks for this!

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

-Kees

> 
> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index 0ea44d975b6c..c5282e62df75 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
> @@ -101,6 +101,21 @@ FIXTURE(tls)
>  	bool notls;
>  };
>  
> +FIXTURE_VARIANT(tls)
> +{
> +	unsigned int tls_version;
> +};
> +
> +FIXTURE_VARIANT_ADD(tls, 12)
> +{
> +	.tls_version = TLS_1_2_VERSION,
> +};
> +
> +FIXTURE_VARIANT_ADD(tls, 13)
> +{
> +	.tls_version = TLS_1_3_VERSION,
> +};
> +
>  FIXTURE_SETUP(tls)
>  {
>  	struct tls12_crypto_info_aes_gcm_128 tls12;
> @@ -112,7 +127,7 @@ FIXTURE_SETUP(tls)
>  	len = sizeof(addr);
>  
>  	memset(&tls12, 0, sizeof(tls12));
> -	tls12.info.version = TLS_1_3_VERSION;
> +	tls12.info.version = variant->tls_version;
>  	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
>  
>  	addr.sin_family = AF_INET;
> @@ -733,7 +748,7 @@ TEST_F(tls, bidir)
>  		struct tls12_crypto_info_aes_gcm_128 tls12;
>  
>  		memset(&tls12, 0, sizeof(tls12));
> -		tls12.info.version = TLS_1_3_VERSION;
> +		tls12.info.version = variant->tls_version;
>  		tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
>  
>  		ret = setsockopt(self->fd, SOL_TLS, TLS_RX, &tls12,
> @@ -1258,78 +1273,4 @@ TEST(keysizes) {
>  	close(cfd);
>  }
>  
> -TEST(tls12) {
> -	int fd, cfd;
> -	bool notls;
> -
> -	struct tls12_crypto_info_aes_gcm_128 tls12;
> -	struct sockaddr_in addr;
> -	socklen_t len;
> -	int sfd, ret;
> -
> -	notls = false;
> -	len = sizeof(addr);
> -
> -	memset(&tls12, 0, sizeof(tls12));
> -	tls12.info.version = TLS_1_2_VERSION;
> -	tls12.info.cipher_type = TLS_CIPHER_AES_GCM_128;
> -
> -	addr.sin_family = AF_INET;
> -	addr.sin_addr.s_addr = htonl(INADDR_ANY);
> -	addr.sin_port = 0;
> -
> -	fd = socket(AF_INET, SOCK_STREAM, 0);
> -	sfd = socket(AF_INET, SOCK_STREAM, 0);
> -
> -	ret = bind(sfd, &addr, sizeof(addr));
> -	ASSERT_EQ(ret, 0);
> -	ret = listen(sfd, 10);
> -	ASSERT_EQ(ret, 0);
> -
> -	ret = getsockname(sfd, &addr, &len);
> -	ASSERT_EQ(ret, 0);
> -
> -	ret = connect(fd, &addr, sizeof(addr));
> -	ASSERT_EQ(ret, 0);
> -
> -	ret = setsockopt(fd, IPPROTO_TCP, TCP_ULP, "tls", sizeof("tls"));
> -	if (ret != 0) {
> -		notls = true;
> -		printf("Failure setting TCP_ULP, testing without tls\n");
> -	}
> -
> -	if (!notls) {
> -		ret = setsockopt(fd, SOL_TLS, TLS_TX, &tls12,
> -				 sizeof(tls12));
> -		ASSERT_EQ(ret, 0);
> -	}
> -
> -	cfd = accept(sfd, &addr, &len);
> -	ASSERT_GE(cfd, 0);
> -
> -	if (!notls) {
> -		ret = setsockopt(cfd, IPPROTO_TCP, TCP_ULP, "tls",
> -				 sizeof("tls"));
> -		ASSERT_EQ(ret, 0);
> -
> -		ret = setsockopt(cfd, SOL_TLS, TLS_RX, &tls12,
> -				 sizeof(tls12));
> -		ASSERT_EQ(ret, 0);
> -	}
> -
> -	close(sfd);
> -
> -	char const *test_str = "test_read";
> -	int send_len = 10;
> -	char buf[10];
> -
> -	send_len = strlen(test_str) + 1;
> -	EXPECT_EQ(send(fd, test_str, send_len, 0), send_len);
> -	EXPECT_NE(recv(cfd, buf, send_len, 0), -1);
> -	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
> -
> -	close(fd);
> -	close(cfd);
> -}
> -
>  TEST_HARNESS_MAIN
> -- 
> 2.24.1
> 

-- 
Kees Cook

Re: [PATCH v3 0/6] kselftest: add fixture parameters

[Kees Cook]

On Mon, Mar 16, 2020 at 03:59:17PM -0700, Jakub Kicinski wrote:
> On Mon, 16 Mar 2020 15:56:40 -0700 Jakub Kicinski wrote:
> > Hi!
> > 
> > Shuah please consider applying to the kselftest tree.
> > 
> > This set is an attempt to make running tests for different
> > sets of data easier. The direct motivation is the tls
> > test which we'd like to run for TLS 1.2 and TLS 1.3,
> > but currently there is no easy way to invoke the same
> > tests with different parameters.
> > 
> > Tested all users of kselftest_harness.h.
> > 
> > v2:
> >  - don't run tests by fixture
> >  - don't pass params as an explicit argument
> > 
> > v3:
> >  - go back to the orginal implementation with an extra
> >    parameter, and running by fixture (Kees);
> >  - add LIST_APPEND helper (Kees);
> >  - add a dot between fixture and param name (Kees);
> >  - rename the params to variants (Tim);
> > 
> > v1: https://lore.kernel.org/netdev/20200313031752.2332565-1-kuba@kernel.org/
> > v2: https://lore.kernel.org/netdev/20200314005501.2446494-1-kuba@kernel.org/
> 
> Ugh, sorry I forgot to realign things after the rename :S
> 
> I'll send a whitespace-only v4 in a hour, allowing a little bit 
> of time in case there are some comments already.

No worries! I think a few small changes are needed for a v5 (please
carry my Acked-bys with for v5). Thanks for working on this; I love it!
:)

-Kees

-- 
Kees Cook