[PATCH v2 0/4] kselftest: add fixture parameters

[PATCH v2 0/4] 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.

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

Note that we loose a little bit of type safety
without passing parameters as an explicit argument.
If user puts the name of the wrong fixture as argument
to CURRENT_FIXTURE() it will happily cast the type.

Jakub Kicinski (4):
  selftests/seccomp: use correct FIXTURE macro
  kselftest: create fixture objects
  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   | 156 ++++++++++++++++--
 tools/testing/selftests/net/tls.c             |  93 ++---------
 tools/testing/selftests/seccomp/seccomp_bpf.c |  10 +-
 4 files changed, 168 insertions(+), 94 deletions(-)

-- 
2.24.1

[PATCH v2 1/4] 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>
Acked-by: Kees Cook <keescook@chromium.org>
---
Kees noted that he posted an equivalent patch already, feel free
to drop this one.
---
 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 v2 2/4] 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>
Acked-by: Kees Cook <keescook@chromium.org>
--
v2:
 - remove the fixture list, we won't iterate over
   fixtures so it's not needed
---
 tools/testing/selftests/kselftest_harness.h | 31 ++++++++++++++++-----
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5336b26506ab..66c2397d8c51 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,6 +214,8 @@
  * 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) \
 	{ \
@@ -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, \
 	 }; \
@@ -632,9 +637,12 @@
 } while (0); OPTIONAL_HANDLER(_assert)
 
 /* Contains all the information for test execution and status checking. */
+struct __fixture_metadata;
+
 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 */
@@ -685,6 +693,13 @@ static inline void __register_test(struct __test_metadata *t)
 	}
 }
 
+/* Contains all the information about a fixture */
+struct __fixture_metadata {
+	const char *name;
+} _fixture_global __attribute__((unused)) = {
+	.name = "global",
+};
+
 static inline int __bail(int for_realz, bool no_print, __u8 step)
 {
 	if (for_realz) {
@@ -695,14 +710,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\n", t->name);
+	printf("[ RUN      ] %s.%s\n", f->name, t->name);
 	alarm(t->timeout);
 	child_pid = fork();
 	if (child_pid < 0) {
@@ -751,7 +767,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"),
+	       f->name, t->name);
 	alarm(0);
 }
 
@@ -768,7 +785,7 @@ static int test_harness_run(int __attribute__((unused)) argc,
 	       __test_count, __fixture_count + 1);
 	for (t = __test_list; t; t = t->next) {
 		count++;
-		__run_test(t);
+		__run_test(t->fixture, t);
 		if (t->passed)
 			pass_count++;
 		else
-- 
2.24.1

[PATCH v2 3/4] kselftest: add fixture parameters

[Jakub Kicinski]

Allow users to pass parameters to fixtures.

Each test will be run once for each set of
its fixture parameter sets (or once if none).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
--
v2:
 - don't pass params to functions, use a member
   of _metadata instead
---
 Documentation/dev-tools/kselftest.rst       |   3 +-
 tools/testing/selftests/kselftest_harness.h | 133 ++++++++++++++++++--
 2 files changed, 126 insertions(+), 10 deletions(-)

diff --git a/Documentation/dev-tools/kselftest.rst b/Documentation/dev-tools/kselftest.rst
index 61ae13c44f91..8aff58d11937 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 CURRENT_PARAMS
 
 Operators
 ---------
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 66c2397d8c51..b7e1ecda441c 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -214,6 +214,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)) \
@@ -246,6 +247,7 @@
 	void fixture_name##_setup( \
 		struct __test_metadata __attribute__((unused)) *_metadata, \
 		FIXTURE_DATA(fixture_name) __attribute__((unused)) *self)
+
 /**
  * FIXTURE_TEARDOWN(fixture_name)
  * *_metadata* is included so that EXPECT_* and ASSERT_* work correctly.
@@ -267,6 +269,72 @@
 		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 accessible in FIXTURE_SETUP(),
+ * FIXTURE_TEARDOWN() and TEST_F(). Tests will be run once 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 =
+
+/**
+ * CURRENT_PARAMS(fixture_name) - Access fixture parameters of the current test
+ *
+ * @fixture_name: fixture name
+ *
+ * .. code-block:: c
+ *
+ *     CURRENT_PARAMS(fixture name)->property1
+ *
+ * Helper macro for accessing parameters of current test. Can be used inside
+ * FIXTURE_SETUP(), FIXTURE_TEARDOWN() and TEST_F().
+ */
+#define CURRENT_PARAMS(fixture_name) \
+	((const FIXTURE_PARAMS(fixture_name) *) _metadata->current_params->data)
+
 /**
  * TEST_F(fixture_name, test_name) - Emits test registration and helpers for
  * fixture-based test cases
@@ -638,11 +706,13 @@
 
 /* Contains all the information for test execution and status checking. */
 struct __fixture_metadata;
+struct __fixture_params_metadata;
 
 struct __test_metadata {
 	const char *name;
 	void (*fn)(struct __test_metadata *);
 	struct __fixture_metadata *fixture;
+	struct __fixture_params_metadata *current_params;
 	int termsig;
 	int passed;
 	int trigger; /* extra handler after the evaluation */
@@ -696,10 +766,41 @@ static inline void __register_test(struct __test_metadata *t)
 /* Contains all the information about a fixture */
 struct __fixture_metadata {
 	const char *name;
+	struct __fixture_params_metadata *params;
 } _fixture_global __attribute__((unused)) = {
 	.name = "global",
 };
 
+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;
+	}
+}
+
 static inline int __bail(int for_realz, bool no_print, __u8 step)
 {
 	if (for_realz) {
@@ -711,14 +812,21 @@ 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;
+
+	t->current_params = p;
+
+	printf("[ RUN      ] %s%s.%s\n", f->name, p->name, t->name);
 	alarm(t->timeout);
 	child_pid = fork();
 	if (child_pid < 0) {
@@ -767,14 +875,17 @@ 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 count = 0;
@@ -784,12 +895,16 @@ static int test_harness_run(int __attribute__((unused)) argc,
 	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->fixture, t);
-		if (t->passed)
-			pass_count++;
-		else
-			ret = 1;
+		f = t->fixture;
+
+		for (p = f->params ?: &no_param; p; p = p->next) {
+			count++;
+			__run_test(f, p, 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 v2 4/4] 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>
Reviewed-by: Kees Cook <keescook@chromium.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..82d48ce0787e 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 = CURRENT_PARAMS(tls)->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 = CURRENT_PARAMS(tls)->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 v2 0/4] kselftest: add fixture parameters

[Kees Cook]

On Fri, Mar 13, 2020 at 05:54:57PM -0700, Jakub Kicinski wrote:
> Note that we loose a little bit of type safety
> without passing parameters as an explicit argument.
> If user puts the name of the wrong fixture as argument
> to CURRENT_FIXTURE() it will happily cast the type.

This got me to take a much closer look at things. I really didn't like
needing to repeat the fixture name in CURRENT_FIXTURE() calls, and then
started coming to all the same conclusions you did in your v1, that I
just didn't quite see yet in my first review. :P

Apologies for my wishy-washy-ness on this, but here's me talking myself
out of my earlier criticisms:

- "I want tests to be run in declaration order" In v1, this is actually
  mostly retained: they're still in declaration order, but they're
  grouped by fixture (which are run in declaration order). That, I think,
  is totally fine. Someone writing code that interleaves between fixtures
  is madness, and having the report retain that ordering seems awful. I
  had thought the declaration ordering was entirely removed, but I see on
  closer inspection that's not true.

- "I'd like everything attached to _metadata" This results in the
  type unsafety you call out here. And I stared at your v2 trying to
  find a way around it, but to get the type attached, it has to be
  part of the __TEST_F_IMPL() glue, and that means passing it along
  side "self", which means plumbing it as a function argument
  everywhere.

So, again, sorry for asking to iterate on v1 instead of v2, though the
v2 _really_ helped me see the problems better. ;)

Something I'd like for v3: instead of "parameters" can we call it
"instances"? It provides a way to run separate instances of the same
fixtures. Those instances have parameters (i.e. struct fields), so I'd
prefer the "instance" naming.

Also a change in reporting:

	struct __fixture_params_metadata no_param = { .name = "", };

Let's make ".name = NULL" here, and then we can detect instantiation:

	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
				p->name ?: "", t->name);

That'll give us single-instance fixtures an unchanged name:

	fixture.test1
	fixture.test2

and instanced fixtures will be:

	fixture.wayA.test1
	fixture.wayA.test2
	fixture.wayB.test1
	fixture.wayB.test2


And finally, since we're in the land of endless macros, I think it
could be possible to make a macro to generate the __register_foo()
routine bodies. By the end of the series there are three nearly identical
functions in the harness for __register_test(), __register_fixture(), and
__register_fixture_instance(). Something like this as an earlier patch to
refactor the __register_test() that can be used by the latter two in their
patches (and counting will likely need to be refactored earlier too):

#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 {						\
		p->next = head;					\
		p->next->prev = item;				\
		p->prev = item;					\
		head = item;					\
	}							\
}

Which should let it be used, ultimately, as:

static inline void __register_test(struct __test_metadata *t)
__LIST_APPEND(__test_list, t)

static inline void __register_fixture(struct __fixture_metadata *f)
__LIST_APPEND(__fixture_list, f)

static inline void
__register_fixture_instance(struct __fixture_metadata *f,
			    struct __fixture_instance_metadata *p)
__LIST_APPEND(f->instances, p)


Thanks for working on this!

-Kees

-- 
Kees Cook

Re: [PATCH v2 0/4] kselftest: add fixture parameters

[David Miller]

From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 13 Mar 2020 17:54:57 -0700

> 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
> 
> Note that we loose a little bit of type safety
> without passing parameters as an explicit argument.
> If user puts the name of the wrong fixture as argument
> to CURRENT_FIXTURE() it will happily cast the type.

Hmmm, what tree should integrate this patch series?

Re: [PATCH v2 0/4] kselftest: add fixture parameters

[Kees Cook]

On Sun, Mar 15, 2020 at 12:05:17AM -0700, David Miller wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Fri, 13 Mar 2020 17:54:57 -0700
> 
> > 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
> > 
> > Note that we loose a little bit of type safety
> > without passing parameters as an explicit argument.
> > If user puts the name of the wrong fixture as argument
> > to CURRENT_FIXTURE() it will happily cast the type.
> 
> Hmmm, what tree should integrate this patch series?

I expect the final version (likely v3) to go via Shuah's selftest tree.

-Kees

-- 
Kees Cook

RE: [PATCH v2 0/4] kselftest: add fixture parameters

[Bird, Tim]

> -----Original Message-----
> From: Kees Cook
> 
> On Fri, Mar 13, 2020 at 05:54:57PM -0700, Jakub Kicinski wrote:
> > Note that we loose a little bit of type safety
> > without passing parameters as an explicit argument.
> > If user puts the name of the wrong fixture as argument
> > to CURRENT_FIXTURE() it will happily cast the type.
> 
> This got me to take a much closer look at things. I really didn't like
> needing to repeat the fixture name in CURRENT_FIXTURE() calls, and then
> started coming to all the same conclusions you did in your v1, that I
> just didn't quite see yet in my first review. :P
> 
> Apologies for my wishy-washy-ness on this, but here's me talking myself
> out of my earlier criticisms:
> 
> - "I want tests to be run in declaration order" In v1, this is actually
>   mostly retained: they're still in declaration order, but they're
>   grouped by fixture (which are run in declaration order). That, I think,
>   is totally fine. Someone writing code that interleaves between fixtures
>   is madness, and having the report retain that ordering seems awful. I
>   had thought the declaration ordering was entirely removed, but I see on
>   closer inspection that's not true.
> 
> - "I'd like everything attached to _metadata" This results in the
>   type unsafety you call out here. And I stared at your v2 trying to
>   find a way around it, but to get the type attached, it has to be
>   part of the __TEST_F_IMPL() glue, and that means passing it along
>   side "self", which means plumbing it as a function argument
>   everywhere.
> 
> So, again, sorry for asking to iterate on v1 instead of v2, though the
> v2 _really_ helped me see the problems better. ;)
> 
> Something I'd like for v3: instead of "parameters" can we call it
> "instances"? It provides a way to run separate instances of the same
> fixtures. Those instances have parameters (i.e. struct fields), so I'd
> prefer the "instance" naming.

Could I humbly suggest "variant" as a possible name here?
IMHO "instance" carries along some semantics related to object
oriented programming, which I think is a bit confusing.  (Maybe that's
intentional though, and you prefer that?)

BTW - Fuego has a similar feature for naming a collection of test
parameters with specific values (if I understand this proposed
feature correctly).  Fuego's feature was named a long time ago
(incorrectly, I think) and it continues to bug me to this day.
It was named 'specs', and after giving it considerable thought
I've been meaning to change it to 'variants'.

Just a suggestion for consideration.  The fact that Fuego got this
wrong is what motivates my suggestion today.  You have to live
with this kind of stuff a long time. :-)

We ran into some issues in Fuego with this concept, that motivate
the comments below.  I'll use your 'instance' terminology in my comments
although the terminology is different in Fuego.

> 
> Also a change in reporting:
> 
> 	struct __fixture_params_metadata no_param = { .name = "", };
> 
> Let's make ".name = NULL" here, and then we can detect instantiation:
> 
> 	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
> 				p->name ?: "", t->name);
> 
> That'll give us single-instance fixtures an unchanged name:
> 
> 	fixture.test1
> 	fixture.test2

We ended up in Fuego adding a 'default' instance name for 
all tests.  That way, all the parsers don't have to be coded to distinguish
if the test identifier includes an instance name or not, which turns
out to be a tough problem.

So single-instance tests would be:
            fixture.default.test1
            fixture.default.test2
> 
> and instanced fixtures will be:
> 
> 	fixture.wayA.test1
> 	fixture.wayA.test2
> 	fixture.wayB.test1
> 	fixture.wayB.test2
> 

Parsing of the test identifiers starts to become a thorny issue 
as you get longer and longer sequences of test-name parts
(test suite, test fixture, sub-test, test-case, measurement, instance, etc.)
It becomes considerably more difficult if  you have more than
one optional element in the identifier, so it's useful to
avoid any optional element you can.

> 
> And finally, since we're in the land of endless macros, I think it
> could be possible to make a macro to generate the __register_foo()
> routine bodies. By the end of the series there are three nearly identical
> functions in the harness for __register_test(), __register_fixture(), and
> __register_fixture_instance(). Something like this as an earlier patch to
> refactor the __register_test() that can be used by the latter two in their
> patches (and counting will likely need to be refactored earlier too):
> 
> #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 {						\
> 		p->next = head;					\
> 		p->next->prev = item;				\
> 		p->prev = item;					\
> 		head = item;					\
> 	}							\
> }
> 
> Which should let it be used, ultimately, as:
> 
> static inline void __register_test(struct __test_metadata *t)
> __LIST_APPEND(__test_list, t)
> 
> static inline void __register_fixture(struct __fixture_metadata *f)
> __LIST_APPEND(__fixture_list, f)
> 
> static inline void
> __register_fixture_instance(struct __fixture_metadata *f,
> 			    struct __fixture_instance_metadata *p)
> __LIST_APPEND(f->instances, p)

With my suggestion of 'variant', this would change to:

static inline void
__register_fixture_variant(struct __fixture_metadata *f,
			    struct __fixture_variant_metadata *p)
__LIST_APPEND(f->variants, p)


Just my 2 cents.
 -- Tim

Re: [PATCH v2 0/4] kselftest: add fixture parameters

[Jakub Kicinski]

On Mon, 16 Mar 2020 15:55:12 +0000 Bird, Tim wrote:
> > -----Original Message-----
> > From: Kees Cook
> > 
> > On Fri, Mar 13, 2020 at 05:54:57PM -0700, Jakub Kicinski wrote:  
> > > Note that we loose a little bit of type safety
> > > without passing parameters as an explicit argument.
> > > If user puts the name of the wrong fixture as argument
> > > to CURRENT_FIXTURE() it will happily cast the type.  
> > 
> > This got me to take a much closer look at things. I really didn't like
> > needing to repeat the fixture name in CURRENT_FIXTURE() calls, and then
> > started coming to all the same conclusions you did in your v1, that I
> > just didn't quite see yet in my first review. :P

No worries, it took me a little bit of internal back and forth to
produce v1, and it's still not at all perfect :S

> > Apologies for my wishy-washy-ness on this, but here's me talking myself
> > out of my earlier criticisms:
> > 
> > - "I want tests to be run in declaration order" In v1, this is actually
> >   mostly retained: they're still in declaration order, but they're
> >   grouped by fixture (which are run in declaration order). That, I think,
> >   is totally fine. Someone writing code that interleaves between fixtures
> >   is madness, and having the report retain that ordering seems awful. I
> >   had thought the declaration ordering was entirely removed, but I see on
> >   closer inspection that's not true.
> > 
> > - "I'd like everything attached to _metadata" This results in the
> >   type unsafety you call out here. And I stared at your v2 trying to
> >   find a way around it, but to get the type attached, it has to be
> >   part of the __TEST_F_IMPL() glue, and that means passing it along
> >   side "self", which means plumbing it as a function argument
> >   everywhere.
> > 
> > So, again, sorry for asking to iterate on v1 instead of v2, though the
> > v2 _really_ helped me see the problems better. ;)
> > 
> > Something I'd like for v3: instead of "parameters" can we call it
> > "instances"? It provides a way to run separate instances of the same
> > fixtures. Those instances have parameters (i.e. struct fields), so I'd
> > prefer the "instance" naming.  
> 
> Could I humbly suggest "variant" as a possible name here?
> IMHO "instance" carries along some semantics related to object
> oriented programming, which I think is a bit confusing.  (Maybe that's
> intentional though, and you prefer that?)

I like parameter or argument, since the data provided to the test 
is constant, and used to guide the instantiation (i.e. "setup"). 
"self" looks more like an instance of a class from OOP point of view.

Variant sounds good too, although the abbreviation would be VAR?
Which isn't ideal. 

But I really don't care so whoever cares the most please speak up :P

> BTW - Fuego has a similar feature for naming a collection of test
> parameters with specific values (if I understand this proposed
> feature correctly).  Fuego's feature was named a long time ago
> (incorrectly, I think) and it continues to bug me to this day.
> It was named 'specs', and after giving it considerable thought
> I've been meaning to change it to 'variants'.
> 
> Just a suggestion for consideration.  The fact that Fuego got this
> wrong is what motivates my suggestion today.  You have to live
> with this kind of stuff a long time. :-)
> 
> We ran into some issues in Fuego with this concept, that motivate
> the comments below.  I'll use your 'instance' terminology in my comments
> although the terminology is different in Fuego.
> 
> > Also a change in reporting:
> > 
> > 	struct __fixture_params_metadata no_param = { .name = "", };
> > 
> > Let's make ".name = NULL" here, and then we can detect instantiation:
> > 
> > 	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
> > 				p->name ?: "", t->name);

Do I have to make it NULL or is it okay to test p->name[0] ?
That way we can save one ternary operator from the litany..

> > That'll give us single-instance fixtures an unchanged name:
> > 
> > 	fixture.test1
> > 	fixture.test2  
> 
> We ended up in Fuego adding a 'default' instance name for 
> all tests.  That way, all the parsers don't have to be coded to distinguish
> if the test identifier includes an instance name or not, which turns
> out to be a tough problem.
> 
> So single-instance tests would be:
>             fixture.default.test1
>             fixture.default.test2

Interesting! That makes sense to me, thanks for sharing the experience.
That's why I just appended the param/instance/variant name to the
fixture name.

To me global.default.XYZ is a mouthful. so in my example (perhaps that
should have been part of the cover letter) I got:

[ RUN      ] global.keysizes             <= non-fixture test
[       OK ] global.keysizes             
[ RUN      ] tls_basic.base_base         <= fixture: "tls_basic", no params
[       OK ] tls_basic.base_base         
[ RUN      ] tls12.sendfile              <= fixture: "tls", param: "12"
[       OK ] tls12.sendfile                 
[ RUN      ] tls13.sendfile              <= fixture: "tls", param: "13"
[       OK ] tls13.sendfile                 (same fixture, diff param)

And users can start inserting underscores themselves if they really
want. (For TLS I was considering different ciphers but they don't impact
testing much.)

> > 
> > and instanced fixtures will be:
> > 
> > 	fixture.wayA.test1
> > 	fixture.wayA.test2
> > 	fixture.wayB.test1
> > 	fixture.wayB.test2
> >   
> 
> Parsing of the test identifiers starts to become a thorny issue 
> as you get longer and longer sequences of test-name parts
> (test suite, test fixture, sub-test, test-case, measurement, instance, etc.)
> It becomes considerably more difficult if  you have more than
> one optional element in the identifier, so it's useful to
> avoid any optional element you can.
> 
> > 
> > And finally, since we're in the land of endless macros, I think it
> > could be possible to make a macro to generate the __register_foo()
> > routine bodies. By the end of the series there are three nearly identical
> > functions in the harness for __register_test(), __register_fixture(), and
> > __register_fixture_instance(). Something like this as an earlier patch to
> > refactor the __register_test() that can be used by the latter two in their
> > patches (and counting will likely need to be refactored earlier too):
> > 
> > #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 {						\
> > 		p->next = head;					\
> > 		p->next->prev = item;				\
> > 		p->prev = item;					\
> > 		head = item;					\
> > 	}							\
> > }
> > 
> > Which should let it be used, ultimately, as:
> > 
> > static inline void __register_test(struct __test_metadata *t)
> > __LIST_APPEND(__test_list, t)
> > 
> > static inline void __register_fixture(struct __fixture_metadata *f)
> > __LIST_APPEND(__fixture_list, f)
> > 
> > static inline void
> > __register_fixture_instance(struct __fixture_metadata *f,
> > 			    struct __fixture_instance_metadata *p)
> > __LIST_APPEND(f->instances, p)  
> 
> With my suggestion of 'variant', this would change to:
> 
> static inline void
> __register_fixture_variant(struct __fixture_metadata *f,
> 			    struct __fixture_variant_metadata *p)
> __LIST_APPEND(f->variants, p)
> 
> 
> Just my 2 cents.
>  -- Tim

Re: [PATCH v2 0/4] kselftest: add fixture parameters

[Kees Cook]

On Mon, Mar 16, 2020 at 01:04:16PM -0700, Jakub Kicinski wrote:
> Variant sounds good too, although the abbreviation would be VAR?
> Which isn't ideal. 
> 
> But I really don't care so whoever cares the most please speak up :P

Let's go with "variant" and just spell it out.

> > BTW - Fuego has a similar feature for naming a collection of test
> > parameters with specific values (if I understand this proposed
> > feature correctly).  Fuego's feature was named a long time ago
> > (incorrectly, I think) and it continues to bug me to this day.
> > It was named 'specs', and after giving it considerable thought
> > I've been meaning to change it to 'variants'.
> > 
> > Just a suggestion for consideration.  The fact that Fuego got this
> > wrong is what motivates my suggestion today.  You have to live
> > with this kind of stuff a long time. :-)
> > 
> > We ran into some issues in Fuego with this concept, that motivate
> > the comments below.  I'll use your 'instance' terminology in my comments
> > although the terminology is different in Fuego.
> > 
> > > Also a change in reporting:
> > > 
> > > 	struct __fixture_params_metadata no_param = { .name = "", };
> > > 
> > > Let's make ".name = NULL" here, and then we can detect instantiation:
> > > 
> > > 	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
> > > 				p->name ?: "", t->name);
> 
> Do I have to make it NULL or is it okay to test p->name[0] ?
> That way we can save one ternary operator from the litany..

I did consider Tim's idea of having them all say 'default', but since
the bulk of tests aren't going to have variants, I don't want to spam
the report with words I have to skip over.

And empty-check (instead of NULL) is fine by me.

> To me global.default.XYZ is a mouthful. so in my example (perhaps that
> should have been part of the cover letter) I got:
> 
> [ RUN      ] global.keysizes             <= non-fixture test
> [       OK ] global.keysizes             
> [ RUN      ] tls_basic.base_base         <= fixture: "tls_basic", no params
> [       OK ] tls_basic.base_base         
> [ RUN      ] tls12.sendfile              <= fixture: "tls", param: "12"
> [       OK ] tls12.sendfile                 
> [ RUN      ] tls13.sendfile              <= fixture: "tls", param: "13"
> [       OK ] tls13.sendfile                 (same fixture, diff param)
> 
> And users can start inserting underscores themselves if they really
> want. (For TLS I was considering different ciphers but they don't impact
> testing much.)

The reason I'd like a dot is just for lay-person grep-ability and
to avoid everyone needing to remember to add separator prefixes --
there should just be a common one. e.g.  searching for "tls13" in the
tree wouldn't find the test (since it's actually named "tls" and "13"
is separate places). (I mean, sure, searching for "tls" is also insane,
but I think I made my point.)

-Kees

-- 
Kees Cook

Re: [PATCH v2 0/4] kselftest: add fixture parameters

[Jakub Kicinski]

On Mon, 16 Mar 2020 14:01:33 -0700 Kees Cook wrote:
> On Mon, Mar 16, 2020 at 01:04:16PM -0700, Jakub Kicinski wrote:
> > Variant sounds good too, although the abbreviation would be VAR?
> > Which isn't ideal. 
> > 
> > But I really don't care so whoever cares the most please speak up :P  
> 
> Let's go with "variant" and just spell it out.
> 
> > > BTW - Fuego has a similar feature for naming a collection of test
> > > parameters with specific values (if I understand this proposed
> > > feature correctly).  Fuego's feature was named a long time ago
> > > (incorrectly, I think) and it continues to bug me to this day.
> > > It was named 'specs', and after giving it considerable thought
> > > I've been meaning to change it to 'variants'.
> > > 
> > > Just a suggestion for consideration.  The fact that Fuego got this
> > > wrong is what motivates my suggestion today.  You have to live
> > > with this kind of stuff a long time. :-)
> > > 
> > > We ran into some issues in Fuego with this concept, that motivate
> > > the comments below.  I'll use your 'instance' terminology in my comments
> > > although the terminology is different in Fuego.
> > >   
> > > > Also a change in reporting:
> > > > 
> > > > 	struct __fixture_params_metadata no_param = { .name = "", };
> > > > 
> > > > Let's make ".name = NULL" here, and then we can detect instantiation:
> > > > 
> > > > 	printf("[ RUN      ] %s%s%s.%s\n", f->name, p->name ? "." : "",
> > > > 				p->name ?: "", t->name);  
> > 
> > Do I have to make it NULL or is it okay to test p->name[0] ?
> > That way we can save one ternary operator from the litany..  
> 
> I did consider Tim's idea of having them all say 'default', but since
> the bulk of tests aren't going to have variants, I don't want to spam
> the report with words I have to skip over.
> 
> And empty-check (instead of NULL) is fine by me.
> 
> > To me global.default.XYZ is a mouthful. so in my example (perhaps that
> > should have been part of the cover letter) I got:
> > 
> > [ RUN      ] global.keysizes             <= non-fixture test
> > [       OK ] global.keysizes             
> > [ RUN      ] tls_basic.base_base         <= fixture: "tls_basic", no params
> > [       OK ] tls_basic.base_base         
> > [ RUN      ] tls12.sendfile              <= fixture: "tls", param: "12"
> > [       OK ] tls12.sendfile                 
> > [ RUN      ] tls13.sendfile              <= fixture: "tls", param: "13"
> > [       OK ] tls13.sendfile                 (same fixture, diff param)
> > 
> > And users can start inserting underscores themselves if they really
> > want. (For TLS I was considering different ciphers but they don't impact
> > testing much.)  
> 
> The reason I'd like a dot is just for lay-person grep-ability and
> to avoid everyone needing to remember to add separator prefixes --
> there should just be a common one. e.g.  searching for "tls13" in the
> tree wouldn't find the test (since it's actually named "tls" and "13"
> is separate places). (I mean, sure, searching for "tls" is also insane,
> but I think I made my point.)

Ack, can't argue with grep-ability :)