[PATCH 0/5] kselftest: add fixture parameters

[PATCH 0/5] kselftest: add fixture parameters

[Jakub Kicinski]

Hi!

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.

Jakub Kicinski (5):
  selftests/seccomp: use correct FIXTURE macro
  kselftest: create fixture objects
  kselftest: run tests by fixture
  kselftest: add fixture parameters
  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   | 228 +++++++++++++++---
 tools/testing/selftests/net/tls.c             |  93 ++-----
 tools/testing/selftests/seccomp/seccomp_bpf.c |  10 +-
 4 files changed, 213 insertions(+), 121 deletions(-)

-- 
2.24.1

[PATCH 1/5] 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

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

[Kees Cook]

On Thu, Mar 12, 2020 at 08:17:48PM -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.

Yup, I ran into this while working on a totally separate series. I sent
a stand-alone patch for this already. (It's identical to this one.)
Shuah can take either one. :)

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

[PATCH 2/5] 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 | 57 +++++++++++++++++----
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5336b26506ab..a396afe4a579 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, \
 	 }; \
@@ -631,10 +636,44 @@
 	} \
 } while (0); OPTIONAL_HANDLER(_assert)
 
+/* 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++;
+	/* Circular linked list where only prev is circular. */
+	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
+		f->next = NULL;
+		f->prev = __fixture_list->prev;
+		f->prev->next = f;
+		__fixture_list->prev = f;
+	} else {
+		f->next = __fixture_list;
+		f->next->prev = f;
+		f->prev = f;
+		__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 */
@@ -647,11 +686,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
@@ -702,7 +736,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) {
@@ -751,7 +785,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

Re: [PATCH 2/5] kselftest: create fixture objects

[Kees Cook]

On Thu, Mar 12, 2020 at 08:17:49PM -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>

I like this!

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

-Kees

> ---
>  tools/testing/selftests/kselftest_harness.h | 57 +++++++++++++++++----
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 5336b26506ab..a396afe4a579 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, \
>  	 }; \
> @@ -631,10 +636,44 @@
>  	} \
>  } while (0); OPTIONAL_HANDLER(_assert)
>  
> +/* 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++;
> +	/* Circular linked list where only prev is circular. */
> +	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
> +		f->next = NULL;
> +		f->prev = __fixture_list->prev;
> +		f->prev->next = f;
> +		__fixture_list->prev = f;
> +	} else {
> +		f->next = __fixture_list;
> +		f->next->prev = f;
> +		f->prev = f;
> +		__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 */
> @@ -647,11 +686,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
> @@ -702,7 +736,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) {
> @@ -751,7 +785,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

[PATCH 3/5] 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 | 42 ++++++++++++---------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index a396afe4a579..7a3392941a5b 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -637,8 +637,11 @@
 } while (0); OPTIONAL_HANDLER(_assert)
 
 /* 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",
@@ -684,7 +687,6 @@ struct __test_metadata {
 };
 
 /* Storage for the (global) tests to be run. */
-static struct __test_metadata *__test_list;
 static unsigned int __test_count;
 
 /*
@@ -698,24 +700,26 @@ static unsigned int __test_count;
  */
 static inline void __register_test(struct __test_metadata *t)
 {
+	struct __fixture_metadata *f = t->fixture;
+
 	__test_count++;
 	/* Circular linked list where only prev is circular. */
-	if (__test_list == NULL) {
-		__test_list = t;
+	if (f->tests == NULL) {
+		f->tests = t;
 		t->next = NULL;
 		t->prev = t;
 		return;
 	}
 	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
 		t->next = NULL;
-		t->prev = __test_list->prev;
+		t->prev = f->tests->prev;
 		t->prev->next = t;
-		__test_list->prev = t;
+		f->tests->prev = t;
 	} else {
-		t->next = __test_list;
+		t->next = f->tests;
 		t->next->prev = t;
 		t->prev = t;
-		__test_list = t;
+		f->tests = t;
 	}
 }
 
@@ -729,14 +733,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) {
@@ -786,13 +791,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;
@@ -801,13 +807,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

Re: [PATCH 3/5] kselftest: run tests by fixture

[Kees Cook]

On Thu, Mar 12, 2020 at 08:17:50PM -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.

I'm not convinced about this change. Declaration order is a pretty
intuitive result that I'd like to keep for the harness.

Can this change be avoided and still keep the final results of a
"mutable" fixture?

-Kees

> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/kselftest_harness.h | 42 ++++++++++++---------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index a396afe4a579..7a3392941a5b 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -637,8 +637,11 @@
>  } while (0); OPTIONAL_HANDLER(_assert)
>  
>  /* 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",
> @@ -684,7 +687,6 @@ struct __test_metadata {
>  };
>  
>  /* Storage for the (global) tests to be run. */
> -static struct __test_metadata *__test_list;
>  static unsigned int __test_count;
>  
>  /*
> @@ -698,24 +700,26 @@ static unsigned int __test_count;
>   */
>  static inline void __register_test(struct __test_metadata *t)
>  {
> +	struct __fixture_metadata *f = t->fixture;
> +
>  	__test_count++;
>  	/* Circular linked list where only prev is circular. */
> -	if (__test_list == NULL) {
> -		__test_list = t;
> +	if (f->tests == NULL) {
> +		f->tests = t;
>  		t->next = NULL;
>  		t->prev = t;
>  		return;
>  	}
>  	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
>  		t->next = NULL;
> -		t->prev = __test_list->prev;
> +		t->prev = f->tests->prev;
>  		t->prev->next = t;
> -		__test_list->prev = t;
> +		f->tests->prev = t;
>  	} else {
> -		t->next = __test_list;
> +		t->next = f->tests;
>  		t->next->prev = t;
>  		t->prev = t;
> -		__test_list = t;
> +		f->tests = t;
>  	}
>  }
>  
> @@ -729,14 +733,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) {
> @@ -786,13 +791,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;
> @@ -801,13 +807,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
> 

-- 
Kees Cook

Re: [PATCH 3/5] kselftest: run tests by fixture

[Jakub Kicinski]

On Fri, 13 Mar 2020 16:27:54 -0700 Kees Cook wrote:
> On Thu, Mar 12, 2020 at 08:17:50PM -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.  
> 
> I'm not convinced about this change. Declaration order is a pretty
> intuitive result that I'd like to keep for the harness.
> 
> Can this change be avoided and still keep the final results of a
> "mutable" fixture?

Sure! I will abandon the reorg of the lists then, keep just the list of
tests, and have each test point to its fixture. Which then may contain
param sets.

[PATCH 4/5] 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>
---
 Documentation/dev-tools/kselftest.rst       |   3 +-
 tools/testing/selftests/kselftest_harness.h | 159 ++++++++++++++++----
 2 files changed, 135 insertions(+), 27 deletions(-)

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index 61ae13c44f91..3c41f7494762 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_PARAMS
+                FIXTURE_PARAMS_ADD
 
 Operators
 ---------
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 7a3392941a5b..78b963f75d3b 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_params_metadata *p) \
+	{ \
+		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_PARAMS(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_PARAMS(fixture_name) __attribute__((unused)) *params)
+
 /**
  * FIXTURE_TEARDOWN(fixture_name)
  * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
@@ -267,6 +276,56 @@
 		struct __test_metadata __attribute__((unused)) *_metadata, \
 		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
 
+/**
+ * FIXTURE_PARAMS(fixture_name) - Optionally called once per fixture
+ * to declare fixture parameters
+ *
+ * @fixture_name: fixture name
+ *
+ * .. code-block:: c
+ *
+ *     FIXTURE_PARAMS(datatype name) {
+ *       type property1;
+ *       ...
+ *     };
+ *
+ * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F()
+ * as *params*.
+ */
+#define FIXTURE_PARAMS(fixture_name) struct _fixture_params_##fixture_name
+
+/**
+ * FIXTURE_PARAMS_ADD(fixture_name, params_name) - Called once per fixture
+ * params to setup the data and register
+ *
+ * @fixture_name: fixture name
+ * @params_name: name of the parameter set
+ *
+ * .. code-block:: c
+ *
+ *     FIXTURE_ADD(datatype name) {
+ *       .property1 = val1;
+ *       ...
+ *     };
+ *
+ * Defines an instance of parameters provided to FIXTURE_SETUP() and TEST_F()
+ * as *params*. Tests of each fixture will be run for each parameter set.
+ */
+#define FIXTURE_PARAMS_ADD(fixture_name, params_name) \
+	extern FIXTURE_PARAMS(fixture_name) \
+		_##fixture_name##_##params_name##_params; \
+	static struct __fixture_params_metadata \
+		_##fixture_name##_##params_name##_object = \
+		{ .name = #params_name, \
+		  .data = &_##fixture_name##_##params_name##_params}; \
+	static void __attribute__((constructor)) \
+		_register_##fixture_name##_##params_name(void) \
+	{ \
+		__register_fixture_params(&_##fixture_name##_fixture_object, \
+			&_##fixture_name##_##params_name##_object);	\
+	} \
+	FIXTURE_PARAMS(fixture_name) _##fixture_name##_##params_name##_params =
+
 /**
  * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
  * fixture-based test cases
@@ -297,18 +356,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_PARAMS(fixture_name) *params); \
 	static inline void wrapper_##fixture_name##_##test_name( \
-		struct __test_metadata *_metadata) \
+		struct __test_metadata *_metadata, \
+		struct __fixture_params_metadata *p) \
 	{ \
 		/* 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, p->data); \
 		/* Let setup failure terminate early. */ \
 		if (!_metadata->passed) \
 			return; \
-		fixture_name##_##test_name(_metadata, &self); \
+		fixture_name##_##test_name(_metadata, &self, p->data); \
 		fixture_name##_teardown(_metadata, &self); \
 	} \
 	static struct __test_metadata \
@@ -326,7 +387,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_PARAMS(fixture_name) __attribute__((unused)) *params)
 
 /**
  * TEST_HARNESS_MAIN - Simple wrapper to run the test harness
@@ -638,10 +700,12 @@
 
 /* Contains all the information about a fixture */
 struct __test_metadata;
+struct __fixture_params_metadata;
 
 struct __fixture_metadata {
 	const char *name;
 	struct __test_metadata *tests;
+	struct __fixture_params_metadata *params;
 	struct __fixture_metadata *prev, *next;
 } _fixture_global __attribute__((unused)) = {
 	.name = "global",
@@ -649,7 +713,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
@@ -657,7 +720,6 @@ static int __constructor_order;
 
 static inline void __register_fixture(struct __fixture_metadata *f)
 {
-	__fixture_count++;
 	/* Circular linked list where only prev is circular. */
 	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
 		f->next = NULL;
@@ -672,10 +734,41 @@ static inline void __register_fixture(struct __fixture_metadata *f)
 	}
 }
 
+struct __fixture_params_metadata {
+	const char *name;
+	const void *data;
+	struct __fixture_params_metadata *prev, *next;
+};
+
+static inline void
+__register_fixture_params(struct __fixture_metadata *f,
+			  struct __fixture_params_metadata *p)
+{
+	/* Circular linked list where only prev is circular. */
+	if (f->params == NULL) {
+		f->params = p;
+		p->next = NULL;
+		p->prev = p;
+		return;
+	}
+	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
+		p->next = NULL;
+		p->prev = f->params->prev;
+		p->prev->next = p;
+		f->params->prev = p;
+	} else {
+		p->next = f->params;
+		p->next->prev = p;
+		p->prev = p;
+		f->params = p;
+	}
+}
+
 /* 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_params_metadata *);
 	struct __fixture_metadata *fixture;
 	int termsig;
 	int passed;
@@ -686,9 +779,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.
@@ -702,7 +792,6 @@ static inline void __register_test(struct __test_metadata *t)
 {
 	struct __fixture_metadata *f = t->fixture;
 
-	__test_count++;
 	/* Circular linked list where only prev is circular. */
 	if (f->tests == NULL) {
 		f->tests = t;
@@ -734,21 +823,26 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
 }
 
 void __run_test(struct __fixture_metadata *f,
+		struct __fixture_params_metadata *p,
 		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\n", f->name, p->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, p);
 		/* return the step that failed or 0 */
 		_exit(t->passed ? 0 : t->step);
 	} else {
@@ -790,31 +884,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\n", (t->passed ? "OK" : "FAIL"),
+	       f->name, p->name, t->name);
 	alarm(0);
 }
 
 static int test_harness_run(int __attribute__((unused)) argc,
 			    char __attribute__((unused)) **argv)
 {
+	struct __fixture_params_metadata no_param = { .name = "", };
+	struct __fixture_params_metadata *p;
 	struct __fixture_metadata *f;
 	struct __test_metadata *t;
 	int ret = 0;
+	unsigned int fixture_count = 0, test_count = 0;
 	unsigned int count = 0;
 	unsigned int pass_count = 0;
 
+	for (f = __fixture_list; f; f = f->next) {
+		fixture_count++;
+		for (p = f->params ?: &no_param; p; p = p->next) {
+			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, fixture_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 (p = f->params ?: &no_param; p; p = p->next) {
+			for (t = f->tests; t; t = t->next) {
+				count++;
+				__run_test(f, p, t);
+				if (t->passed)
+					pass_count++;
+				else
+					ret = 1;
+			}
 		}
 	}
 	printf("[==========] %u / %u tests passed.\n", pass_count, count);
-- 
2.24.1

Re: [PATCH 4/5] kselftest: add fixture parameters

[Kees Cook]

On Thu, Mar 12, 2020 at 08:17:51PM -0700, Jakub Kicinski wrote:
> 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>
> ---
>  Documentation/dev-tools/kselftest.rst       |   3 +-
>  tools/testing/selftests/kselftest_harness.h | 159 ++++++++++++++++----
>  2 files changed, 135 insertions(+), 27 deletions(-)
> 
> diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
> index 61ae13c44f91..3c41f7494762 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_PARAMS
> +                FIXTURE_PARAMS_ADD
>  
>  Operators
>  ---------
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 7a3392941a5b..78b963f75d3b 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_params_metadata *p) \
> +	{ \
> +		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_PARAMS(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_PARAMS(fixture_name) __attribute__((unused)) *params)
> +
>  /**
>   * FIXTURE_TEARDOWN(fixture_name)
>   * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
> @@ -267,6 +276,56 @@
>  		struct __test_metadata __attribute__((unused)) *_metadata, \
>  		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
>  
> +/**
> + * FIXTURE_PARAMS(fixture_name) - Optionally called once per fixture
> + * to declare fixture parameters
> + *
> + * @fixture_name: fixture name
> + *
> + * .. code-block:: c
> + *
> + *     FIXTURE_PARAMS(datatype name) {
> + *       type property1;
> + *       ...
> + *     };
> + *
> + * Defines type of constant parameters provided to FIXTURE_SETUP() and TEST_F()
> + * as *params*.
> + */
> +#define FIXTURE_PARAMS(fixture_name) struct _fixture_params_##fixture_name
> +
> +/**
> + * FIXTURE_PARAMS_ADD(fixture_name, params_name) - Called once per fixture
> + * params to setup the data and register
> + *
> + * @fixture_name: fixture name
> + * @params_name: name of the parameter set
> + *
> + * .. code-block:: c
> + *
> + *     FIXTURE_ADD(datatype name) {
> + *       .property1 = val1;
> + *       ...
> + *     };
> + *
> + * Defines an instance of parameters provided to FIXTURE_SETUP() and TEST_F()
> + * as *params*. Tests of each fixture will be run for each parameter set.
> + */
> +#define FIXTURE_PARAMS_ADD(fixture_name, params_name) \
> +	extern FIXTURE_PARAMS(fixture_name) \
> +		_##fixture_name##_##params_name##_params; \
> +	static struct __fixture_params_metadata \
> +		_##fixture_name##_##params_name##_object = \
> +		{ .name = #params_name, \
> +		  .data = &_##fixture_name##_##params_name##_params}; \
> +	static void __attribute__((constructor)) \
> +		_register_##fixture_name##_##params_name(void) \
> +	{ \
> +		__register_fixture_params(&_##fixture_name##_fixture_object, \
> +			&_##fixture_name##_##params_name##_object);	\
> +	} \
> +	FIXTURE_PARAMS(fixture_name) _##fixture_name##_##params_name##_params =
> +
>  /**
>   * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
>   * fixture-based test cases
> @@ -297,18 +356,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_PARAMS(fixture_name) *params); \
>  	static inline void wrapper_##fixture_name##_##test_name( \
> -		struct __test_metadata *_metadata) \
> +		struct __test_metadata *_metadata, \
> +		struct __fixture_params_metadata *p) \
>  	{ \
>  		/* 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, p->data); \
>  		/* Let setup failure terminate early. */ \
>  		if (!_metadata->passed) \
>  			return; \
> -		fixture_name##_##test_name(_metadata, &self); \
> +		fixture_name##_##test_name(_metadata, &self, p->data); \
>  		fixture_name##_teardown(_metadata, &self); \
>  	} \
>  	static struct __test_metadata \
> @@ -326,7 +387,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_PARAMS(fixture_name) __attribute__((unused)) *params)

Could this be done without expanding the function arguments? (i.e. can
the params just stay attached to the __test_metadata, perhaps having the
test runner adjust a new "current_param" variable to point to the
current param? Having everything attached to the single __test_metadata
makes a lot of things easier, IMO.

-Kees

>  
>  /**
>   * TEST_HARNESS_MAIN - Simple wrapper to run the test harness
> @@ -638,10 +700,12 @@
>  
>  /* Contains all the information about a fixture */
>  struct __test_metadata;
> +struct __fixture_params_metadata;
>  
>  struct __fixture_metadata {
>  	const char *name;
>  	struct __test_metadata *tests;
> +	struct __fixture_params_metadata *params;
>  	struct __fixture_metadata *prev, *next;
>  } _fixture_global __attribute__((unused)) = {
>  	.name = "global",
> @@ -649,7 +713,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
> @@ -657,7 +720,6 @@ static int __constructor_order;
>  
>  static inline void __register_fixture(struct __fixture_metadata *f)
>  {
> -	__fixture_count++;
>  	/* Circular linked list where only prev is circular. */
>  	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
>  		f->next = NULL;
> @@ -672,10 +734,41 @@ static inline void __register_fixture(struct __fixture_metadata *f)
>  	}
>  }
>  
> +struct __fixture_params_metadata {
> +	const char *name;
> +	const void *data;
> +	struct __fixture_params_metadata *prev, *next;
> +};
> +
> +static inline void
> +__register_fixture_params(struct __fixture_metadata *f,
> +			  struct __fixture_params_metadata *p)
> +{
> +	/* Circular linked list where only prev is circular. */
> +	if (f->params == NULL) {
> +		f->params = p;
> +		p->next = NULL;
> +		p->prev = p;
> +		return;
> +	}
> +	if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {
> +		p->next = NULL;
> +		p->prev = f->params->prev;
> +		p->prev->next = p;
> +		f->params->prev = p;
> +	} else {
> +		p->next = f->params;
> +		p->next->prev = p;
> +		p->prev = p;
> +		f->params = p;
> +	}
> +}
> +
>  /* 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_params_metadata *);
>  	struct __fixture_metadata *fixture;
>  	int termsig;
>  	int passed;
> @@ -686,9 +779,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.
> @@ -702,7 +792,6 @@ static inline void __register_test(struct __test_metadata *t)
>  {
>  	struct __fixture_metadata *f = t->fixture;
>  
> -	__test_count++;
>  	/* Circular linked list where only prev is circular. */
>  	if (f->tests == NULL) {
>  		f->tests = t;
> @@ -734,21 +823,26 @@ static inline int __bail(int for_realz, bool no_print, __u8 step)
>  }
>  
>  void __run_test(struct __fixture_metadata *f,
> +		struct __fixture_params_metadata *p,
>  		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\n", f->name, p->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, p);
>  		/* return the step that failed or 0 */
>  		_exit(t->passed ? 0 : t->step);
>  	} else {
> @@ -790,31 +884,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\n", (t->passed ? "OK" : "FAIL"),
> +	       f->name, p->name, t->name);
>  	alarm(0);
>  }
>  
>  static int test_harness_run(int __attribute__((unused)) argc,
>  			    char __attribute__((unused)) **argv)
>  {
> +	struct __fixture_params_metadata no_param = { .name = "", };
> +	struct __fixture_params_metadata *p;
>  	struct __fixture_metadata *f;
>  	struct __test_metadata *t;
>  	int ret = 0;
> +	unsigned int fixture_count = 0, test_count = 0;
>  	unsigned int count = 0;
>  	unsigned int pass_count = 0;
>  
> +	for (f = __fixture_list; f; f = f->next) {
> +		fixture_count++;
> +		for (p = f->params ?: &no_param; p; p = p->next) {
> +			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, fixture_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 (p = f->params ?: &no_param; p; p = p->next) {
> +			for (t = f->tests; t; t = t->next) {
> +				count++;
> +				__run_test(f, p, 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 4/5] kselftest: add fixture parameters

[Jakub Kicinski]

On Fri, 13 Mar 2020 16:31:25 -0700 Kees Cook wrote:
> > @@ -326,7 +387,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_PARAMS(fixture_name) __attribute__((unused)) *params)  
> 
> Could this be done without expanding the function arguments? (i.e. can
> the params just stay attached to the __test_metadata, perhaps having the
> test runner adjust a new "current_param" variable to point to the
> current param? Having everything attached to the single __test_metadata
> makes a lot of things easier, IMO.

Sure! I felt a little awkward dereferencing _metadata in the test,
so I followed the example of self. But I can change.

Can I add a macro like CURRENT_PARAM() that would implicitly use
_metadata?

Re: [PATCH 4/5] kselftest: add fixture parameters

[Kees Cook]

On Fri, Mar 13, 2020 at 04:52:02PM -0700, Jakub Kicinski wrote:
> On Fri, 13 Mar 2020 16:31:25 -0700 Kees Cook wrote:
> > > @@ -326,7 +387,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_PARAMS(fixture_name) __attribute__((unused)) *params)  
> > 
> > Could this be done without expanding the function arguments? (i.e. can
> > the params just stay attached to the __test_metadata, perhaps having the
> > test runner adjust a new "current_param" variable to point to the
> > current param? Having everything attached to the single __test_metadata
> > makes a lot of things easier, IMO.
> 
> Sure! I felt a little awkward dereferencing _metadata in the test,
> so I followed the example of self. But I can change.
> 
> Can I add a macro like CURRENT_PARAM() that would implicitly use
> _metadata?

Yeah, that seems cleaner. Thanks! This is very cool. :)

-- 
Kees Cook

[PATCH 5/5] 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..63029728ac97 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -101,6 +101,21 @@ FIXTURE(tls)
 	bool notls;
 };
 
+FIXTURE_PARAMS(tls)
+{
+	unsigned int tls_version;
+};
+
+FIXTURE_PARAMS_ADD(tls, 12)
+{
+	.tls_version = TLS_1_2_VERSION,
+};
+
+FIXTURE_PARAMS_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 = params->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 = params->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 5/5] selftests: tls: run all tests for TLS 1.2 and TLS 1.3

[Kees Cook]

On Thu, Mar 12, 2020 at 08:17:52PM -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>

I really like the resulting effect here.

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

-Kees

> ---
>  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..63029728ac97 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
> @@ -101,6 +101,21 @@ FIXTURE(tls)
>  	bool notls;
>  };
>  
> +FIXTURE_PARAMS(tls)
> +{
> +	unsigned int tls_version;
> +};
> +
> +FIXTURE_PARAMS_ADD(tls, 12)
> +{
> +	.tls_version = TLS_1_2_VERSION,
> +};
> +
> +FIXTURE_PARAMS_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 = params->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 = params->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