[PATCH RFC 0/3] selftests/nolibc: introduce new test harness

[PATCH RFC 0/3] selftests/nolibc: introduce new test harness

[Thomas Weißschuh]

This series introduces a new test harness for nolibc.
It is similar to kselftest-harness and google test.
More information in patch 1.

This is an RFC to gather feedback, especially if it can be integrated
with the original kselftest-harness somehow.

Note:

When run under qemu-loongarch64 8.1.2 the test "mmap_munmap_good" fails.
This is a bug in qemu-user and already fixed there on master.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (3):
      selftests/nolibc: add custom test harness
      selftests/nolibc: migrate startup tests to new harness
      selftests/nolibc: migrate vfprintf tests to new harness

 tools/testing/selftests/nolibc/nolibc-harness.h | 269 ++++++++++++++++++++++++
 tools/testing/selftests/nolibc/nolibc-test.c    | 180 ++++++++--------
 2 files changed, 353 insertions(+), 96 deletions(-)
---
base-commit: d38d5366cb1c51f687b4720277adee97074b22e9
change-id: 20231105-nolibc-harness-28c59029d7a5

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH RFC 1/3] selftests/nolibc: add custom test harness

[Thomas Weißschuh]

The harness provides a framework to write unit tests for nolibc itself
and kernel selftests using nolibc.

Advantages over the current harness:
* Makes it possible to emit KTAP for integration into kselftests.
* Provides familiarity with the kselftest harness and google test.
* It is nicer to write testcases that are longer than one line.

Design goals:
* Compatibility with nolibc. kselftest-harness requires setjmp() and
  signals which are not supported on nolibc.
* Provide the same output as the existing unittests.
* Provide a way to emit KTAP.

Notes:
* This differs from kselftest-harness in its support for test suites,
  the same as google test.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/nolibc-harness.h | 269 ++++++++++++++++++++++++
 1 file changed, 269 insertions(+)

diff --git a/tools/testing/selftests/nolibc/nolibc-harness.h b/tools/testing/selftests/nolibc/nolibc-harness.h
new file mode 100644
index 000000000000..4c82581fab86
--- /dev/null
+++ b/tools/testing/selftests/nolibc/nolibc-harness.h
@@ -0,0 +1,269 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Test harness for NOLIBC
+ *
+ * Copyright (C) 2023 Thomas Weißschuh <linux@weissschuh.net>
+ * Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
+ */
+
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+
+static void putcharn(char c, size_t n)
+{
+	char buf[64];
+
+	memset(buf, c, n);
+	buf[n] = '\0';
+	fputs(buf, stdout);
+}
+
+struct __test_setup;
+
+struct __test_execution {
+	int finished, failed, skipped, llen;
+};
+
+struct __test_metadata {
+	const char *name;
+	const char *suite;
+	unsigned int suite_count;
+	void (*func)(struct __test_metadata *metadata);
+	struct __test_metadata *next;
+	struct __test_setup *setup;
+	struct __test_execution exe;
+};
+
+struct __test_setup {
+	struct __test_metadata *start, *end;
+} __test_setup __attribute__((weak));
+
+static void __add_metadata(struct __test_metadata *metadata)
+{
+	struct __test_metadata *m;
+
+	if (!__test_setup.start)
+		__test_setup.start = metadata;
+
+	if (!__test_setup.end) {
+		__test_setup.end = metadata;
+	} else {
+		__test_setup.end->next = metadata;
+		__test_setup.end = metadata;
+	}
+
+	metadata->setup = &__test_setup;
+
+	for (m = __test_setup.start; m; m = m->next) {
+		if (strcmp(metadata->suite, m->suite) == 0)
+			metadata->suite_count++;
+	}
+}
+
+#define TEST(_suite, _name) \
+	static void _testfunc_ ## _suite ## _name(struct __test_metadata *); \
+	static struct __test_metadata _metadata_ ## _suite ## _name = { \
+		.name = #_name, \
+		.suite = #_suite, \
+		.func = _testfunc_ ## _suite ## _name, \
+	}; \
+	__attribute__((constructor)) \
+	static void _register_testfunc_ ## _suite ## _name(void) \
+	{ __add_metadata(&_metadata_ ## _suite ## _name); } \
+	static void _testfunc_ ## _suite ## _name( \
+		struct __test_metadata *_metadata __attribute__((unused)))
+
+#define SKIP(statement) __extension__ ({ \
+	_metadata->exe.skipped = 1; \
+	statement; \
+})
+
+#define FAIL(statement) __extension__ ({ \
+	_metadata->exe.failed = 1; \
+	statement; \
+})
+
+static void __run_test(struct __test_metadata *metadata)
+{
+	metadata->func(metadata);
+	metadata->exe.finished = 1;
+}
+
+static __attribute__((unused))
+unsigned int count_test_suites(void)
+{
+	struct __test_metadata *m;
+	unsigned int r = 0;
+
+	for (m = __test_setup.start; m && m->suite_count; m = m->next)
+		r++;
+
+	return r;
+}
+
+static __attribute__((unused))
+int count_tests(void)
+{
+	struct __test_metadata *m;
+	unsigned int r = 0;
+
+	for (m = __test_setup.start; m; m = m->next)
+		r++;
+
+	return r;
+}
+
+static unsigned int count_suite_tests(const char *suite)
+{
+	struct __test_metadata *m;
+	unsigned int c = 0;
+
+	for (m = __test_setup.start; m && m->suite_count; m = m->next)
+		if (strcmp(m->suite, suite) == 0)
+			c = m->suite_count;
+
+	return c;
+}
+
+static __attribute__((unused))
+void dump_test_plan(void)
+{
+	struct __test_metadata *m;
+	const char *suite = "";
+
+	printf("PLAN:\n");
+	for (m = __test_setup.start; m; m = m->next) {
+		if (strcmp(suite, m->suite)) {
+			suite = m->suite;
+			printf("  Suite %s (%d):\n", suite, count_suite_tests(suite));
+		}
+		printf("    %10s:%s %d\n", m->suite, m->name, m->suite_count - 1);
+	}
+}
+
+static unsigned int run_test_suite(const char *suite, int min, int max)
+{
+	struct __test_metadata *m;
+	const char *status;
+	unsigned int errors = 0;
+	int printed;
+
+	for (m = __test_setup.start; m; m = m->next) {
+		int testnum = m->suite_count - 1;
+
+		if (strcmp(suite, m->suite) == 0 && testnum >= min && testnum <= max) {
+			printed = printf("%d %s", testnum, m->name);
+			__run_test(m);
+			printed += m->exe.llen;
+			if (printed < 64)
+				putcharn(' ', 64 - printed);
+
+			if (m->exe.failed)
+				status = " [FAIL]";
+			else if (m->exe.skipped)
+				status = "[SKIPPED]";
+			else
+				status = "  [OK]";
+
+			printf("%s\n", status);
+			if (m->exe.failed)
+				errors++;
+		}
+	}
+	return errors;
+};
+
+static __attribute__((unused))
+void reset_tests(void)
+{
+	struct __test_metadata *m;
+
+	for (m = __test_setup.start; m; m = m->next)
+		memset(&m->exe, 0, sizeof(m->exe));
+}
+
+static __attribute__((unused))
+unsigned int run_all_tests(void)
+{
+	struct __test_metadata *m;
+	unsigned int suite_errors, errors = 0;
+
+	for (m = __test_setup.start; m; m = m->next) {
+		if (!m->exe.finished) {
+			printf("Running test '%s'\n", m->suite);
+			suite_errors = run_test_suite(m->suite, 0, INT_MAX);
+			printf("Errors during this test: %d\n\n", suite_errors);
+			errors += suite_errors;
+		}
+	}
+
+	printf("Total number of errors: %d\n", errors);
+	return errors;
+}
+
+#define ASSERT_EQ(expected, seen) \
+	__ASSERT(expected, #expected, seen, #seen, ==)
+#define ASSERT_NE(expected, seen) \
+	__ASSERT(expected, #expected, seen, #seen, !=)
+#define ASSERT_LT(expected, seen) \
+	__ASSERT(expected, #expected, seen, #seen, <)
+#define ASSERT_LE(expected, seen) \
+	__ASSERT(expected, #expected, seen, #seen, <=)
+#define ASSERT_GT(expected, seen) \
+	__ASSERT(expected, #expected, seen, #seen, >)
+#define ASSERT_GE(expected, seen) \
+	__ASSERT(expected, #expected, seen, #seen, >=)
+#define ASSERT_NULL(seen) \
+	__ASSERT(NULL, "NULL", seen, #seen, ==)
+#define ASSERT_TRUE(seen) \
+	__ASSERT(0, "0", seen, #seen, !=)
+#define ASSERT_FALSE(seen) \
+	__ASSERT(0, "0", seen, #seen, ==)
+
+#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
+#define is_pointer_type(var)	(__builtin_classify_type(var) == 5)
+
+#define __ASSERT(_expected, _expected_str, _seen, _seen_str, _t) __extension__ ({ \
+	/* Avoid multiple evaluation of the cases */ \
+	__typeof__(_expected) __exp = (_expected); \
+	__typeof__(_seen) __seen = (_seen); \
+	int __ok = __exp _t __seen; \
+	if (!__ok) \
+		_metadata->exe.failed = 1; \
+	if (is_pointer_type(__exp)) { \
+		void * __expected_print = (void *)(uintptr_t)__exp; \
+		_metadata->exe.llen = printf(" = <%p> ", __expected_print); \
+	} else if (is_signed_type(__exp)) { \
+		long long __expected_print = (intptr_t)__exp; \
+		_metadata->exe.llen = printf(" = %lld ", __expected_print); \
+	} else { \
+		unsigned long long __expected_print = (uintptr_t)__exp; \
+		_metadata->exe.llen = printf(" = %llu ", __expected_print); \
+	} \
+	__ok; \
+})
+
+#define ASSERT_STREQ(expected, seen) \
+	__ASSERT_STR(expected, seen, ==)
+#define ASSERT_STRNE(expected, seen) \
+	__ASSERT_STR(expected, seen, !=)
+
+#define __ASSERT_STR(_expected, _seen, _t) __extension__ ({ \
+	const char *__exp = (_expected); \
+	const char *__seen = (_seen); \
+	int __ok = __seen && __exp && strcmp(__exp, __seen) _t 0; \
+	if (!__ok) \
+		_metadata->exe.failed = 1; \
+	_metadata->exe.llen = printf(" = <%s> ", __exp ? __exp : ""); \
+	__ok; \
+})
+
+#define ASSERT_STRNZ(seen) __extension__ ({ \
+	const char *__seen = (seen); \
+	int __ok = !!__seen; \
+	if (!__ok) \
+		_metadata->exe.failed = 1; \
+	_metadata->exe.llen = printf(" = <%s> ", __seen); \
+	__ok; \
+})

-- 
2.42.1

Re: [PATCH RFC 1/3] selftests/nolibc: add custom test harness

[Willy Tarreau]

Hi Thomas,

On Wed, Nov 15, 2023 at 10:08:19PM +0100, Thomas Weißschuh wrote:
> The harness provides a framework to write unit tests for nolibc itself
> and kernel selftests using nolibc.
> 
> Advantages over the current harness:
> * Makes it possible to emit KTAP for integration into kselftests.
> * Provides familiarity with the kselftest harness and google test.
> * It is nicer to write testcases that are longer than one line.
> 
> Design goals:
> * Compatibility with nolibc. kselftest-harness requires setjmp() and
>   signals which are not supported on nolibc.
> * Provide the same output as the existing unittests.
> * Provide a way to emit KTAP.
> 
> Notes:
> * This differs from kselftest-harness in its support for test suites,
>   the same as google test.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Nice intro to present the benefits, but you forgot to explain what
the patch itself does among these points, the decisions you took,
tradeoffs if any etc. All of these are particularly important so as
to figure what to expect from the patch itself, because, tob be
honest, for me it's a bit difficult to estimate the suitability of
the code for a given purpose, thus for now I'll mostly focus on
general code.

A few comments below:

> +static void putcharn(char c, size_t n)
> +{
> +	char buf[64];
> +
> +	memset(buf, c, n);
> +	buf[n] = '\0';
> +	fputs(buf, stdout);
> +}

You should really check that n < 64 here, not only because it's test
code that will trigger about any possible bug around, but also because
you want others to easily contribute and not get trapped by calling
this with a larger value without figuring it will do whatever. And
that way you can remove the tests from the callers which don't need
to hard-code this limit.

> +#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
> +#define is_pointer_type(var)	(__builtin_classify_type(var) == 5)

The hard-coded "5" above should either be replaced with pointer_type_class
(if available here) or left as-is with a comment at the end of the line
saying e.g. "// pointer_type_class" so that the value can be looked up
more easily if needed.

Willy

Re: [PATCH RFC 1/3] selftests/nolibc: add custom test harness

[Thomas Weißschuh]

On 2023-11-16 08:16:22+0100, Willy Tarreau wrote:
> Hi Thomas,
> 
> On Wed, Nov 15, 2023 at 10:08:19PM +0100, Thomas Weißschuh wrote:
> > The harness provides a framework to write unit tests for nolibc itself
> > and kernel selftests using nolibc.
> > 
> > Advantages over the current harness:
> > * Makes it possible to emit KTAP for integration into kselftests.
> > * Provides familiarity with the kselftest harness and google test.
> > * It is nicer to write testcases that are longer than one line.
> > 
> > Design goals:
> > * Compatibility with nolibc. kselftest-harness requires setjmp() and
> >   signals which are not supported on nolibc.
> > * Provide the same output as the existing unittests.
> > * Provide a way to emit KTAP.
> > 
> > Notes:
> > * This differs from kselftest-harness in its support for test suites,
> >   the same as google test.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> Nice intro to present the benefits, but you forgot to explain what
> the patch itself does among these points, the decisions you took,
> tradeoffs if any etc. All of these are particularly important so as
> to figure what to expect from the patch itself, because, tob be
> honest, for me it's a bit difficult to estimate the suitability of
> the code for a given purpose, thus for now I'll mostly focus on
> general code.

Good points. I'll expand more in v2 after we are through this round.

> A few comments below:
> 
> > +static void putcharn(char c, size_t n)
> > +{
> > +	char buf[64];
> > +
> > +	memset(buf, c, n);
> > +	buf[n] = '\0';
> > +	fputs(buf, stdout);
> > +}
> 
> You should really check that n < 64 here, not only because it's test
> code that will trigger about any possible bug around, but also because
> you want others to easily contribute and not get trapped by calling
> this with a larger value without figuring it will do whatever. And
> that way you can remove the tests from the callers which don't need
> to hard-code this limit.

Ack.

> 
> > +#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
> > +#define is_pointer_type(var)	(__builtin_classify_type(var) == 5)
> 
> The hard-coded "5" above should either be replaced with pointer_type_class
> (if available here) or left as-is with a comment at the end of the line
> saying e.g. "// pointer_type_class" so that the value can be looked up
> more easily if needed.

Ack.

[PATCH RFC 2/3] selftests/nolibc: migrate startup tests to new harness

[Thomas Weißschuh]

Migrate part of nolibc-test.c to the new test harness.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 106 ++++++++++++++-------------
 1 file changed, 56 insertions(+), 50 deletions(-)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index e173014f6b66..6c1b42b58e3e 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -42,6 +42,7 @@
 #endif
 
 #include "nolibc-test-linkage.h"
+#include "nolibc-harness.h"
 
 /* for the type of int_fast16_t and int_fast32_t, musl differs from glibc and nolibc */
 #define SINT_MAX_OF_TYPE(type) (((type)1 << (sizeof(type) * 8 - 2)) - (type)1 + ((type)1 << (sizeof(type) * 8 - 2)))
@@ -130,15 +131,6 @@ static const char *errorname(int err)
 	}
 }
 
-static void putcharn(char c, size_t n)
-{
-	char buf[64];
-
-	memset(buf, c, n);
-	buf[n] = '\0';
-	fputs(buf, stdout);
-}
-
 enum RESULT {
 	OK,
 	FAIL,
@@ -599,6 +591,19 @@ int expect_strne(const char *expr, int llen, const char *cmp)
 #define CASE_TEST(name) \
 	case __LINE__: llen += printf("%d %s", test, #name);
 
+#if defined(NOLIBC)
+
+#define ASSUME_NOLIBC(stmt)
+
+#else /* defined(NOLIBC) */
+
+/* differ from nolibc, both glibc and musl have no global _auxv */
+unsigned long *_auxv = (void *)-1;
+#define ASSUME_NOLIBC(stmt) SKIP(stmt)
+
+#endif /* defined(NOLIBC) */
+
+
 /* constructors validate that they are executed in definition order */
 __attribute__((constructor))
 static void constructor1(void)
@@ -612,53 +617,54 @@ static void constructor2(void)
 	constructor_test_value *= 2;
 }
 
-int run_startup(int min, int max)
+static const void *pbrk(void)
 {
-	int test;
-	int ret = 0;
-	/* kernel at least passes HOME and TERM, shell passes more */
-	int env_total = 2;
-	/* checking NULL for argv/argv0, environ and _auxv is not enough, let's compare with sbrk(0) or &end */
 	extern char end;
-	char *brk = sbrk(0) != (void *)-1 ? sbrk(0) : &end;
-	/* differ from nolibc, both glibc and musl have no global _auxv */
-	const unsigned long *test_auxv = (void *)-1;
-#ifdef NOLIBC
-	test_auxv = _auxv;
-#endif
+	static char *brk;
 
-	for (test = min; test >= 0 && test <= max; test++) {
-		int llen = 0; /* line length */
+	if (brk)
+		return brk;
 
-		/* avoid leaving empty lines below, this will insert holes into
-		 * test numbers.
-		 */
-		switch (test + __LINE__ + 1) {
-		CASE_TEST(argc);             EXPECT_GE(1, test_argc, 1); break;
-		CASE_TEST(argv_addr);        EXPECT_PTRGT(1, test_argv, brk); break;
-		CASE_TEST(argv_environ);     EXPECT_PTRLT(1, test_argv, environ); break;
-		CASE_TEST(argv_total);       EXPECT_EQ(1, environ - test_argv - 1, test_argc ?: 1); break;
-		CASE_TEST(argv0_addr);       EXPECT_PTRGT(1, argv0, brk); break;
-		CASE_TEST(argv0_str);        EXPECT_STRNZ(1, argv0 > brk ? argv0 : NULL); break;
-		CASE_TEST(argv0_len);        EXPECT_GE(1,  argv0 > brk ? strlen(argv0) : 0, 1); break;
-		CASE_TEST(environ_addr);     EXPECT_PTRGT(1, environ, brk); break;
-		CASE_TEST(environ_envp);     EXPECT_PTREQ(1, environ, test_envp); break;
-		CASE_TEST(environ_auxv);     EXPECT_PTRLT(test_auxv != (void *)-1, environ, test_auxv); break;
-		CASE_TEST(environ_total);    EXPECT_GE(test_auxv != (void *)-1, (void *)test_auxv - (void *)environ - 1, env_total); break;
-		CASE_TEST(environ_HOME);     EXPECT_PTRNZ(1, getenv("HOME")); break;
-		CASE_TEST(auxv_addr);        EXPECT_PTRGT(test_auxv != (void *)-1, test_auxv, brk); break;
-		CASE_TEST(auxv_AT_UID);      EXPECT_EQ(1, getauxval(AT_UID), getuid()); break;
-		CASE_TEST(constructor);      EXPECT_EQ(1, constructor_test_value, 2); break;
-		CASE_TEST(linkage_errno);    EXPECT_PTREQ(1, linkage_test_errno_addr(), &errno); break;
-		CASE_TEST(linkage_constr);   EXPECT_EQ(1, linkage_test_constructor_test_value, 6); break;
-		case __LINE__:
-			return ret; /* must be last */
-		/* note: do not set any defaults so as to permit holes above */
-		}
-	}
-	return ret;
+	brk = sbrk(0);
+
+	if (brk == (void *)-1)
+		brk = &end;
+
+	return brk;
 }
 
+TEST(startup, argc)           { ASSERT_GE(test_argc, 1); }
+TEST(startup, argv_addr)      { ASSERT_GT((void *)test_argv, pbrk()); }
+TEST(startup, argv_environ)   { ASSERT_LT(test_argv, environ); }
+TEST(startup, argv_total)     { ASSERT_EQ(environ - test_argv - 1, test_argc ?: 1); }
+TEST(startup, argv0_addr)     { ASSERT_GT((void *)argv0, pbrk()); }
+TEST(startup, argv0_str)      { ASSERT_STRNZ((void *)argv0 > pbrk() ? argv0 : NULL); }
+TEST(startup, argv0_len)      { ASSERT_GE((void *)argv0 > pbrk() ? strlen(argv0) : 0U, 1U); }
+TEST(startup, environ_addr)   { ASSERT_GT((void *)environ, pbrk()); }
+TEST(startup, environ_envp)   { ASSERT_EQ(environ, test_envp); }
+TEST(startup, environ_auxv)   {
+	ASSUME_NOLIBC(return);
+	ASSERT_LT((void *)environ, (void *)_auxv);
+}
+TEST(startup, environ_total)  {
+	ASSUME_NOLIBC(return);
+	/* kernel at least passes HOME and TERM, shell passes more */
+	ASSERT_GE((void *)_auxv - (void *)environ - 1, 2);
+}
+TEST(startup, environ_HOME)   { ASSERT_NE(getenv("HOME"), NULL); }
+TEST(startup, auxv_addr)      {
+	ASSUME_NOLIBC(return);
+	ASSERT_GT((void *)_auxv, pbrk());
+}
+TEST(startup, auxv_AT_UID)    { ASSERT_EQ(getauxval(AT_UID), getuid()); }
+TEST(startup, constructor)    { ASSERT_EQ(constructor_test_value, 2); }
+TEST(startup, linkage_errno)  { ASSERT_EQ(linkage_test_errno_addr(), &errno); }
+TEST(startup, linkage_constr) { ASSERT_EQ(linkage_test_constructor_test_value, 6); }
+
+int run_startup(int min, int max)
+{
+	return run_test_suite("startup", min, max);
+}
 
 /* used by some syscall tests below */
 int test_getdents64(const char *dir)

-- 
2.42.1

Re: [PATCH RFC 2/3] selftests/nolibc: migrate startup tests to new harness

[Willy Tarreau]

On Wed, Nov 15, 2023 at 10:08:20PM +0100, Thomas Weißschuh wrote:
> Migrate part of nolibc-test.c to the new test harness.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

A few points, mostly questions and food for thoughts.

> -static void putcharn(char c, size_t n)
> -{
> -	char buf[64];
> -
> -	memset(buf, c, n);
> -	buf[n] = '\0';
> -	fputs(buf, stdout);
> -}
> -

Ah now I see how the other one came from :-)  My comment about the size
check still stands anyway, especially when placed in an include file.

> +#if defined(NOLIBC)
> +
> +#define ASSUME_NOLIBC(stmt)
> +
> +#else /* defined(NOLIBC) */
> +
> +/* differ from nolibc, both glibc and musl have no global _auxv */
> +unsigned long *_auxv = (void *)-1;
> +#define ASSUME_NOLIBC(stmt) SKIP(stmt)
> +
> +#endif /* defined(NOLIBC) */
> +

I've seen below how it's used and don't find this very clear. In general,
passing a statement as an argument to a macro, especially control statements
such as "return" is a bit difficult to grasp. If the macro is only used for
this, maybe it should integrate the return statement and be called something
like "RETURN_UNLESS_NOLIBC()" which is quite explicit this time. If you really
need to keep the statement adjustable, then most likely that calling the
macro "UNLESS_NOLIBC()" would help, because I understand more naturally
that the following will perform a return if we're not on nolibc:

    UNLESS_NOLIBC(return);

than:

    ASSUME_NOLIBC(return);

> -	for (test = min; test >= 0 && test <= max; test++) {
> -		int llen = 0; /* line length */
> +	if (brk)
> +		return brk;
>  
> -		/* avoid leaving empty lines below, this will insert holes into
> -		 * test numbers.
> -		 */
> -		switch (test + __LINE__ + 1) {
> -		CASE_TEST(argc);             EXPECT_GE(1, test_argc, 1); break;
> -		CASE_TEST(argv_addr);        EXPECT_PTRGT(1, test_argv, brk); break;
> -		CASE_TEST(argv_environ);     EXPECT_PTRLT(1, test_argv, environ); break;
> -		CASE_TEST(argv_total);       EXPECT_EQ(1, environ - test_argv - 1, test_argc ?: 1); break;
> -		CASE_TEST(argv0_addr);       EXPECT_PTRGT(1, argv0, brk); break;
> -		CASE_TEST(argv0_str);        EXPECT_STRNZ(1, argv0 > brk ? argv0 : NULL); break;
> -		CASE_TEST(argv0_len);        EXPECT_GE(1,  argv0 > brk ? strlen(argv0) : 0, 1); break;
> -		CASE_TEST(environ_addr);     EXPECT_PTRGT(1, environ, brk); break;
> -		CASE_TEST(environ_envp);     EXPECT_PTREQ(1, environ, test_envp); break;
> -		CASE_TEST(environ_auxv);     EXPECT_PTRLT(test_auxv != (void *)-1, environ, test_auxv); break;
> -		CASE_TEST(environ_total);    EXPECT_GE(test_auxv != (void *)-1, (void *)test_auxv - (void *)environ - 1, env_total); break;
> -		CASE_TEST(environ_HOME);     EXPECT_PTRNZ(1, getenv("HOME")); break;
> -		CASE_TEST(auxv_addr);        EXPECT_PTRGT(test_auxv != (void *)-1, test_auxv, brk); break;
> -		CASE_TEST(auxv_AT_UID);      EXPECT_EQ(1, getauxval(AT_UID), getuid()); break;
> -		CASE_TEST(constructor);      EXPECT_EQ(1, constructor_test_value, 2); break;
> -		CASE_TEST(linkage_errno);    EXPECT_PTREQ(1, linkage_test_errno_addr(), &errno); break;
> -		CASE_TEST(linkage_constr);   EXPECT_EQ(1, linkage_test_constructor_test_value, 6); break;
> -		case __LINE__:
> -			return ret; /* must be last */
> -		/* note: do not set any defaults so as to permit holes above */
> -		}
> -	}
> -	return ret;
> +	brk = sbrk(0);
> +
> +	if (brk == (void *)-1)
> +		brk = &end;
> +
> +	return brk;
>  }
>  
> +TEST(startup, argc)           { ASSERT_GE(test_argc, 1); }
> +TEST(startup, argv_addr)      { ASSERT_GT((void *)test_argv, pbrk()); }
> +TEST(startup, argv_environ)   { ASSERT_LT(test_argv, environ); }
> +TEST(startup, argv_total)     { ASSERT_EQ(environ - test_argv - 1, test_argc ?: 1); }
> +TEST(startup, argv0_addr)     { ASSERT_GT((void *)argv0, pbrk()); }
> +TEST(startup, argv0_str)      { ASSERT_STRNZ((void *)argv0 > pbrk() ? argv0 : NULL); }
> +TEST(startup, argv0_len)      { ASSERT_GE((void *)argv0 > pbrk() ? strlen(argv0) : 0U, 1U); }
> +TEST(startup, environ_addr)   { ASSERT_GT((void *)environ, pbrk()); }
> +TEST(startup, environ_envp)   { ASSERT_EQ(environ, test_envp); }
> +TEST(startup, environ_auxv)   {
> +	ASSUME_NOLIBC(return);
> +	ASSERT_LT((void *)environ, (void *)_auxv);
> +}
> +TEST(startup, environ_total)  {
> +	ASSUME_NOLIBC(return);
> +	/* kernel at least passes HOME and TERM, shell passes more */
> +	ASSERT_GE((void *)_auxv - (void *)environ - 1, 2);
> +}
> +TEST(startup, environ_HOME)   { ASSERT_NE(getenv("HOME"), NULL); }
> +TEST(startup, auxv_addr)      {
> +	ASSUME_NOLIBC(return);
> +	ASSERT_GT((void *)_auxv, pbrk());
> +}
> +TEST(startup, auxv_AT_UID)    { ASSERT_EQ(getauxval(AT_UID), getuid()); }
> +TEST(startup, constructor)    { ASSERT_EQ(constructor_test_value, 2); }
> +TEST(startup, linkage_errno)  { ASSERT_EQ(linkage_test_errno_addr(), &errno); }
> +TEST(startup, linkage_constr) { ASSERT_EQ(linkage_test_constructor_test_value, 6); }

I do appreciate the much lower indent level that still manages to
enumerate tests easily. But given that test suites are grouped, shouldn't
we go a bit further and state that TEST() operates on the suite defined
by the TEST_SUITE macro that must be defined before it ? This way you would
have:

  #define TEST_SUITE startup
  TEST(argc)           { ASSERT_GE(test_argc, 1); }
  TEST(argv_addr)      { ASSERT_GT((void *)test_argv, pbrk()); }
  ...
  #undef TEST_SUITE

One thing that was not immediately obvious to me upon first read was
if TEST() defines or executes a test (i.e. "test" is both a noun and a
verb). Of course, spending 10 more seconds on the patch makes it obvious
it's a definition, but maybe following the same logic we have with
run_test_suite(), we should place the verb in front, for example
"DEF_TEST()" which then makes it quite unambiguous. Any opinion ?

Willy

Re: [PATCH RFC 2/3] selftests/nolibc: migrate startup tests to new harness

[Thomas Weißschuh]

On 2023-11-16 08:33:27+0100, Willy Tarreau wrote:
> On Wed, Nov 15, 2023 at 10:08:20PM +0100, Thomas Weißschuh wrote:
> > Migrate part of nolibc-test.c to the new test harness.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> A few points, mostly questions and food for thoughts.
> 
> > -static void putcharn(char c, size_t n)
> > -{
> > -	char buf[64];
> > -
> > -	memset(buf, c, n);
> > -	buf[n] = '\0';
> > -	fputs(buf, stdout);
> > -}
> > -
> 
> Ah now I see how the other one came from :-)  My comment about the size
> check still stands anyway, especially when placed in an include file.
> 
> > +#if defined(NOLIBC)
> > +
> > +#define ASSUME_NOLIBC(stmt)
> > +
> > +#else /* defined(NOLIBC) */
> > +
> > +/* differ from nolibc, both glibc and musl have no global _auxv */
> > +unsigned long *_auxv = (void *)-1;
> > +#define ASSUME_NOLIBC(stmt) SKIP(stmt)
> > +
> > +#endif /* defined(NOLIBC) */
> > +
> 
> I've seen below how it's used and don't find this very clear. In general,
> passing a statement as an argument to a macro, especially control statements
> such as "return" is a bit difficult to grasp. If the macro is only used for
> this, maybe it should integrate the return statement and be called something
> like "RETURN_UNLESS_NOLIBC()" which is quite explicit this time. If you really
> need to keep the statement adjustable, then most likely that calling the
> macro "UNLESS_NOLIBC()" would help, because I understand more naturally
> that the following will perform a return if we're not on nolibc:
> 
>     UNLESS_NOLIBC(return);
> 
> than:
> 
>     ASSUME_NOLIBC(return);

The statement arguments is modelled after SKIP() from
kselftest_harness.h.

But the wrapper you proposed is indeed much better,
I'll switch to that.

> 
> > -	for (test = min; test >= 0 && test <= max; test++) {
> > -		int llen = 0; /* line length */
> > +	if (brk)
> > +		return brk;
> >  
> > -		/* avoid leaving empty lines below, this will insert holes into
> > -		 * test numbers.
> > -		 */
> > -		switch (test + __LINE__ + 1) {
> > -		CASE_TEST(argc);             EXPECT_GE(1, test_argc, 1); break;
> > -		CASE_TEST(argv_addr);        EXPECT_PTRGT(1, test_argv, brk); break;
> > -		CASE_TEST(argv_environ);     EXPECT_PTRLT(1, test_argv, environ); break;
> > -		CASE_TEST(argv_total);       EXPECT_EQ(1, environ - test_argv - 1, test_argc ?: 1); break;
> > -		CASE_TEST(argv0_addr);       EXPECT_PTRGT(1, argv0, brk); break;
> > -		CASE_TEST(argv0_str);        EXPECT_STRNZ(1, argv0 > brk ? argv0 : NULL); break;
> > -		CASE_TEST(argv0_len);        EXPECT_GE(1,  argv0 > brk ? strlen(argv0) : 0, 1); break;
> > -		CASE_TEST(environ_addr);     EXPECT_PTRGT(1, environ, brk); break;
> > -		CASE_TEST(environ_envp);     EXPECT_PTREQ(1, environ, test_envp); break;
> > -		CASE_TEST(environ_auxv);     EXPECT_PTRLT(test_auxv != (void *)-1, environ, test_auxv); break;
> > -		CASE_TEST(environ_total);    EXPECT_GE(test_auxv != (void *)-1, (void *)test_auxv - (void *)environ - 1, env_total); break;
> > -		CASE_TEST(environ_HOME);     EXPECT_PTRNZ(1, getenv("HOME")); break;
> > -		CASE_TEST(auxv_addr);        EXPECT_PTRGT(test_auxv != (void *)-1, test_auxv, brk); break;
> > -		CASE_TEST(auxv_AT_UID);      EXPECT_EQ(1, getauxval(AT_UID), getuid()); break;
> > -		CASE_TEST(constructor);      EXPECT_EQ(1, constructor_test_value, 2); break;
> > -		CASE_TEST(linkage_errno);    EXPECT_PTREQ(1, linkage_test_errno_addr(), &errno); break;
> > -		CASE_TEST(linkage_constr);   EXPECT_EQ(1, linkage_test_constructor_test_value, 6); break;
> > -		case __LINE__:
> > -			return ret; /* must be last */
> > -		/* note: do not set any defaults so as to permit holes above */
> > -		}
> > -	}
> > -	return ret;
> > +	brk = sbrk(0);
> > +
> > +	if (brk == (void *)-1)
> > +		brk = &end;
> > +
> > +	return brk;
> >  }
> >  
> > +TEST(startup, argc)           { ASSERT_GE(test_argc, 1); }
> > +TEST(startup, argv_addr)      { ASSERT_GT((void *)test_argv, pbrk()); }
> > +TEST(startup, argv_environ)   { ASSERT_LT(test_argv, environ); }
> > +TEST(startup, argv_total)     { ASSERT_EQ(environ - test_argv - 1, test_argc ?: 1); }
> > +TEST(startup, argv0_addr)     { ASSERT_GT((void *)argv0, pbrk()); }
> > +TEST(startup, argv0_str)      { ASSERT_STRNZ((void *)argv0 > pbrk() ? argv0 : NULL); }
> > +TEST(startup, argv0_len)      { ASSERT_GE((void *)argv0 > pbrk() ? strlen(argv0) : 0U, 1U); }
> > +TEST(startup, environ_addr)   { ASSERT_GT((void *)environ, pbrk()); }
> > +TEST(startup, environ_envp)   { ASSERT_EQ(environ, test_envp); }
> > +TEST(startup, environ_auxv)   {
> > +	ASSUME_NOLIBC(return);
> > +	ASSERT_LT((void *)environ, (void *)_auxv);
> > +}
> > +TEST(startup, environ_total)  {
> > +	ASSUME_NOLIBC(return);
> > +	/* kernel at least passes HOME and TERM, shell passes more */
> > +	ASSERT_GE((void *)_auxv - (void *)environ - 1, 2);
> > +}
> > +TEST(startup, environ_HOME)   { ASSERT_NE(getenv("HOME"), NULL); }
> > +TEST(startup, auxv_addr)      {
> > +	ASSUME_NOLIBC(return);
> > +	ASSERT_GT((void *)_auxv, pbrk());
> > +}
> > +TEST(startup, auxv_AT_UID)    { ASSERT_EQ(getauxval(AT_UID), getuid()); }
> > +TEST(startup, constructor)    { ASSERT_EQ(constructor_test_value, 2); }
> > +TEST(startup, linkage_errno)  { ASSERT_EQ(linkage_test_errno_addr(), &errno); }
> > +TEST(startup, linkage_constr) { ASSERT_EQ(linkage_test_constructor_test_value, 6); }
> 
> I do appreciate the much lower indent level that still manages to
> enumerate tests easily. But given that test suites are grouped, shouldn't
> we go a bit further and state that TEST() operates on the suite defined
> by the TEST_SUITE macro that must be defined before it ? This way you would
> have:
> 
>   #define TEST_SUITE startup
>   TEST(argc)           { ASSERT_GE(test_argc, 1); }
>   TEST(argv_addr)      { ASSERT_GT((void *)test_argv, pbrk()); }
>   ...
>   #undef TEST_SUITE
> 
> One thing that was not immediately obvious to me upon first read was
> if TEST() defines or executes a test (i.e. "test" is both a noun and a
> verb). Of course, spending 10 more seconds on the patch makes it obvious
> it's a definition, but maybe following the same logic we have with
> run_test_suite(), we should place the verb in front, for example
> "DEF_TEST()" which then makes it quite unambiguous. Any opinion ?

The TEST() macro is modelled after kselftest_harness
(which only takes one argument, as it doesn't support suites)
and google test which works the same as the new TEST().

So I would prefer to keep the name.

As for specifying the suite via a macro:
I like that it saves even more indentation but at the same time it feels
a bit too implicit.

I'm not sure...

[PATCH RFC 3/3] selftests/nolibc: migrate vfprintf tests to new harness

[Thomas Weißschuh]

Migrate part of nolibc-test.c to the new test harness.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++-----------------
 1 file changed, 28 insertions(+), 46 deletions(-)

diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
index 6c1b42b58e3e..c0e7e090a05a 100644
--- a/tools/testing/selftests/nolibc/nolibc-test.c
+++ b/tools/testing/selftests/nolibc/nolibc-test.c
@@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max)
 	return ret;
 }
 
-#define EXPECT_VFPRINTF(c, expected, fmt, ...)				\
-	ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
+#define ASSERT_VFPRINTF(c, expected, fmt, ...)				\
+	enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \
+	if (res == SKIPPED) SKIP(return); \
+	if (res == FAIL) FAIL(return);
 
-static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...)
+static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c,
+				   const char *expected, const char *fmt, ...)
 {
 	int ret, fd;
 	ssize_t w, r;
@@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm
 	va_list args;
 
 	fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600);
-	if (fd == -1) {
-		result(llen, SKIPPED);
-		return 0;
-	}
+	if (fd == -1)
+		return SKIPPED;
 
 	memfile = fdopen(fd, "w+");
-	if (!memfile) {
-		result(llen, FAIL);
-		return 1;
-	}
+	if (!memfile)
+		return FAIL;
 
 	va_start(args, fmt);
 	w = vfprintf(memfile, fmt, args);
 	va_end(args);
 
 	if (w != c) {
-		llen += printf(" written(%d) != %d", (int)w, c);
-		result(llen, FAIL);
-		return 1;
+		_metadata->exe.llen += printf(" written(%d) != %d", (int)w, c);
+		return FAIL;
 	}
 
 	fflush(memfile);
@@ -1080,46 +1078,30 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm
 	fclose(memfile);
 
 	if (r != w) {
-		llen += printf(" written(%d) != read(%d)", (int)w, (int)r);
-		result(llen, FAIL);
-		return 1;
+		_metadata->exe.llen += printf(" written(%d) != read(%d)", (int)w, (int)r);
+		return FAIL;
 	}
 
 	buf[r] = '\0';
-	llen += printf(" \"%s\" = \"%s\"", expected, buf);
+	_metadata->exe.llen += printf(" \"%s\" = \"%s\"", expected, buf);
 	ret = strncmp(expected, buf, c);
 
-	result(llen, ret ? FAIL : OK);
-	return ret;
+	return ret ? FAIL : OK;
 }
 
-static int run_vfprintf(int min, int max)
+TEST(vfprintf, empty)     { ASSERT_VFPRINTF(0, "", ""); }
+TEST(vfprintf, simple)    { ASSERT_VFPRINTF(3, "foo", "foo"); }
+TEST(vfprintf, string)    { ASSERT_VFPRINTF(3, "foo", "%s", "foo"); }
+TEST(vfprintf, number)    { ASSERT_VFPRINTF(4, "1234", "%d", 1234); }
+TEST(vfprintf, negnumber) { ASSERT_VFPRINTF(5, "-1234", "%d", -1234); }
+TEST(vfprintf, unsigned)  { ASSERT_VFPRINTF(5, "12345", "%u", 12345); }
+TEST(vfprintf, char)      { ASSERT_VFPRINTF(1, "c", "%c", 'c'); }
+TEST(vfprintf, hex)       { ASSERT_VFPRINTF(1, "f", "%x", 0xf); }
+TEST(vfprintf, pointer)   { ASSERT_VFPRINTF(3, "0x1", "%p", (void *) 0x1); }
+
+int run_vfprintf(int min, int max)
 {
-	int test;
-	int ret = 0;
-
-	for (test = min; test >= 0 && test <= max; test++) {
-		int llen = 0; /* line length */
-
-		/* avoid leaving empty lines below, this will insert holes into
-		 * test numbers.
-		 */
-		switch (test + __LINE__ + 1) {
-		CASE_TEST(empty);        EXPECT_VFPRINTF(0, "", ""); break;
-		CASE_TEST(simple);       EXPECT_VFPRINTF(3, "foo", "foo"); break;
-		CASE_TEST(string);       EXPECT_VFPRINTF(3, "foo", "%s", "foo"); break;
-		CASE_TEST(number);       EXPECT_VFPRINTF(4, "1234", "%d", 1234); break;
-		CASE_TEST(negnumber);    EXPECT_VFPRINTF(5, "-1234", "%d", -1234); break;
-		CASE_TEST(unsigned);     EXPECT_VFPRINTF(5, "12345", "%u", 12345); break;
-		CASE_TEST(char);         EXPECT_VFPRINTF(1, "c", "%c", 'c'); break;
-		CASE_TEST(hex);          EXPECT_VFPRINTF(1, "f", "%x", 0xf); break;
-		CASE_TEST(pointer);      EXPECT_VFPRINTF(3, "0x1", "%p", (void *) 0x1); break;
-		case __LINE__:
-			return ret; /* must be last */
-		/* note: do not set any defaults so as to permit holes above */
-		}
-	}
-	return ret;
+	return run_test_suite("vfprintf", min, max);
 }
 
 static int smash_stack(void)

-- 
2.42.1

Re: [PATCH RFC 3/3] selftests/nolibc: migrate vfprintf tests to new harness

[Willy Tarreau]

On Wed, Nov 15, 2023 at 10:08:21PM +0100, Thomas Weißschuh wrote:
> Migrate part of nolibc-test.c to the new test harness.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++-----------------
>  1 file changed, 28 insertions(+), 46 deletions(-)
> 
> diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> index 6c1b42b58e3e..c0e7e090a05a 100644
> --- a/tools/testing/selftests/nolibc/nolibc-test.c
> +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max)
>  	return ret;
>  }
>  
> -#define EXPECT_VFPRINTF(c, expected, fmt, ...)				\
> -	ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
> +#define ASSERT_VFPRINTF(c, expected, fmt, ...)				\
> +	enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \
> +	if (res == SKIPPED) SKIP(return); \
> +	if (res == FAIL) FAIL(return);

Please enclose this in a "do { } while (0)" block when using more than
one statement, because you can be certain that sooner or later someone
will put an "if" or "for" above it. This will also avoid the double
colon caused by the trailing one.

> -static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...)
> +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c,
> +				   const char *expected, const char *fmt, ...)
>  {
>  	int ret, fd;
>  	ssize_t w, r;
> @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm
>  	va_list args;
>  
>  	fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600);
> -	if (fd == -1) {
> -		result(llen, SKIPPED);
> -		return 0;
> -	}
> +	if (fd == -1)
> +		return SKIPPED;
>  
>  	memfile = fdopen(fd, "w+");
> -	if (!memfile) {
> -		result(llen, FAIL);
> -		return 1;
> -	}
> +	if (!memfile)
> +		return FAIL;
 

Till now it looks easier and more readable.

>  	va_start(args, fmt);
>  	w = vfprintf(memfile, fmt, args);
>  	va_end(args);
>  
>  	if (w != c) {
> -		llen += printf(" written(%d) != %d", (int)w, c);
> -		result(llen, FAIL);
> -		return 1;
> +		_metadata->exe.llen += printf(" written(%d) != %d", (int)w, c);
> +		return FAIL;
>  	}

Here however I feel like we're already hacking internals of the test
system and that doesn't seem natural nor logical. OK I understand that
llen contains the lenght of the emitted message, but how should the
user easily guess that it's placed into ->exe.llen, and they may or may
not touch other fields there, etc ? Also the fact that the variable is
prefixed with an underscore signals a warning to the user that they
should not fiddle too much with its internals.

I'm seeing basically two possible approaches:
  - one consisting in having a wrapper around printf() that transparently
    sets the llen field in _metadata->exe. This at least offload this
    knowledge from the user who can then just know they have to pass an
    opaque metadata argument and that's all.

  - one consisting in saying that such functions should not affect the
    test's metadata themselves and that it ought to be the caller's job
    instead. The function then only ought to return the printed length,
    and will not need the metadata at all.

I tend to prefer the second option. In addition, you seem to be returning
only 3 statuses (ok/skip/fail) so returning a single composite value for
this is easy, e.g. (result | llen) with result made of macros only touching
the high bits. If in the future more returns are needed, either a larger
int or shift can be used, or we can then return pairs or structs.

Regards,
Willy

Re: [PATCH RFC 3/3] selftests/nolibc: migrate vfprintf tests to new harness

[Thomas Weißschuh]

On 2023-11-16 08:48:02+0100, Willy Tarreau wrote:
> On Wed, Nov 15, 2023 at 10:08:21PM +0100, Thomas Weißschuh wrote:
> > Migrate part of nolibc-test.c to the new test harness.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  tools/testing/selftests/nolibc/nolibc-test.c | 74 +++++++++++-----------------
> >  1 file changed, 28 insertions(+), 46 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > index 6c1b42b58e3e..c0e7e090a05a 100644
> > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > @@ -1039,10 +1039,13 @@ int run_stdlib(int min, int max)
> >  	return ret;
> >  }
> >  
> > -#define EXPECT_VFPRINTF(c, expected, fmt, ...)				\
> > -	ret += expect_vfprintf(llen, c, expected, fmt, ##__VA_ARGS__)
> > +#define ASSERT_VFPRINTF(c, expected, fmt, ...)				\
> > +	enum RESULT res = assert_vfprintf(_metadata, c, expected, fmt, ##__VA_ARGS__); \
> > +	if (res == SKIPPED) SKIP(return); \
> > +	if (res == FAIL) FAIL(return);
> 
> Please enclose this in a "do { } while (0)" block when using more than
> one statement, because you can be certain that sooner or later someone
> will put an "if" or "for" above it. This will also avoid the double
> colon caused by the trailing one.

Ack.

> 
> > -static int expect_vfprintf(int llen, int c, const char *expected, const char *fmt, ...)
> > +static enum RESULT assert_vfprintf(struct __test_metadata *_metadata, int c,
> > +				   const char *expected, const char *fmt, ...)
> >  {
> >  	int ret, fd;
> >  	ssize_t w, r;
> > @@ -1051,25 +1054,20 @@ static int expect_vfprintf(int llen, int c, const char *expected, const char *fm
> >  	va_list args;
> >  
> >  	fd = open("/tmp", O_TMPFILE | O_EXCL | O_RDWR, 0600);
> > -	if (fd == -1) {
> > -		result(llen, SKIPPED);
> > -		return 0;
> > -	}
> > +	if (fd == -1)
> > +		return SKIPPED;
> >  
> >  	memfile = fdopen(fd, "w+");
> > -	if (!memfile) {
> > -		result(llen, FAIL);
> > -		return 1;
> > -	}
> > +	if (!memfile)
> > +		return FAIL;
>  
> 
> Till now it looks easier and more readable.
> 
> >  	va_start(args, fmt);
> >  	w = vfprintf(memfile, fmt, args);
> >  	va_end(args);
> >  
> >  	if (w != c) {
> > -		llen += printf(" written(%d) != %d", (int)w, c);
> > -		result(llen, FAIL);
> > -		return 1;
> > +		_metadata->exe.llen += printf(" written(%d) != %d", (int)w, c);
> > +		return FAIL;
> >  	}
> 
> Here however I feel like we're already hacking internals of the test
> system and that doesn't seem natural nor logical. OK I understand that
> llen contains the lenght of the emitted message, but how should the
> user easily guess that it's placed into ->exe.llen, and they may or may
> not touch other fields there, etc ? Also the fact that the variable is
> prefixed with an underscore signals a warning to the user that they
> should not fiddle too much with its internals.

Agree that this is ugly.

> I'm seeing basically two possible approaches:
>   - one consisting in having a wrapper around printf() that transparently
>     sets the llen field in _metadata->exe. This at least offload this
>     knowledge from the user who can then just know they have to pass an
>     opaque metadata argument and that's all.
> 
>   - one consisting in saying that such functions should not affect the
>     test's metadata themselves and that it ought to be the caller's job
>     instead. The function then only ought to return the printed length,
>     and will not need the metadata at all.
> 
> I tend to prefer the second option. In addition, you seem to be returning
> only 3 statuses (ok/skip/fail) so returning a single composite value for
> this is easy, e.g. (result | llen) with result made of macros only touching
> the high bits. If in the future more returns are needed, either a larger
> int or shift can be used, or we can then return pairs or structs.

I am prefering the first option. It will make it easier to adapt the
backend of the harness to KTAP I think.

If you are fine with the basics of the harness I can convert all of
nolibc-test.c and then also add the KTAP output at the end.

WDYT?


Thomas