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