Re: [PATCH] lib: stackinit: Convert to KUnit

[Daniel Latypov <dlatypov@google.com>]

On Wed, Feb 23, 2022 at 9:51 PM Kees Cook <keescook@chromium.org> wrote:
>

<snip>


>  /* Userspace headers. */
> +#define _GNU_SOURCE
>  #include <stdio.h>
>  #include <stdint.h>
> +#include <stdlib.h>
>  #include <string.h>
>  #include <stdbool.h>
>  #include <errno.h>
>  #include <sys/types.h>
>
>  /* Linux kernel-ism stubs for stand-alone userspace build. */

This is neat and esp. so that it works.
But may I ask, what's the value of using this vs UML?

Given this has changed into mainly just a KUnit-compatibility layer,
it feels like it can maybe live as a standalone file, if there's ever
interest in doing this for other tests.

It feels like something that will never quite be "supported", but I
find it neat enough I'd have fun sending some patches to make it more
realistic.

> -#define KBUILD_MODNAME         "stackinit"
> -#define pr_fmt(fmt)            KBUILD_MODNAME ": " fmt
> -#define pr_err(fmt, ...)       fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_warn(fmt, ...)      fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__)
> -#define pr_info(fmt, ...)      fprintf(stdout, pr_fmt(fmt), ##__VA_ARGS__)
> -#define __init                 /**/
> -#define __exit                 /**/
> +#define TEST_PASS      0
> +#define TEST_SKIP      1
> +#define TEST_FAIL      2
> +struct kunit {
> +       int status;
> +       char *msg;
> +};
> +struct kunit_case {
> +        void (*run_case)(struct kunit *test);
> +        const char *name;
> +};
> +struct kunit_suite {
> +       const char *name;
> +       const struct kunit_case *test_cases;
> +};
> +#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> +
> +#define KUNIT_ASSERT_TRUE_MSG(test, expr, fmt, ...)                    \
> +do {                                                                   \
> +       if (!(expr)) {                                                  \
> +               if (test->status != TEST_SKIP)                          \
> +                       test->status = TEST_FAIL;                       \
> +               if (test->msg)                                          \
> +                       free(test->msg);                                \
> +               asprintf(&test->msg, fmt, ##__VA_ARGS__);               \
> +       }                                                               \
> +} while (0)

This looks more like KUNIT_EXPECT_TRUE_MSG(), since this macro won't
abort the test if the expectation fails.

Looking at the code, it looks like we do want the ability to abort.
Perhaps we can do what Googletest does in C++ and just add in a `return;`?

It has some annoying implications, like using them from helper
functions doesn't work as one would expect.
But people seem to be doing fine with that tradeoff in C++ land.

> +
> +#define KUNIT_ASSERT_EQ_MSG(test, left, right, fmt, ...)               \
> +       KUNIT_ASSERT_TRUE_MSG(test, (left) == (right), fmt, ##__VA_ARGS__)

Very optional:

It might be nice to show the expressions automatically on failure.
We could implement that via something like

KUNIT_ASSERT_TRUE_MSG(test, (left) == (right), #left " != " #right ":
" fmt, ##__VA_ARGS__);

E.g.
KUNIT_ASSERT_EQ_MSG(test, 2+2, 5, "math is broken")
=> 2+2 != 5: math is broken

But I can see that being a bit too complicated to want to do here.
And the failure messages we had before are already decent at giving context.

> +
> +#define kunit_skip(test, fmt, ...)                                     \
> +do {                                                                   \
> +       test->status = TEST_SKIP;                                       \
> +       if (test->msg)                                                  \
> +               free(test->msg);                                        \
> +       asprintf(&test->msg, fmt, ##__VA_ARGS__);                       \
> +} while (0)

Similarly, this has no control flow implications, so the current
semantics match kunit_mark_skipped().

But looking at the code, I think we want to abort early here too.

> +
>  #define __user                 /**/
>  #define noinline               __attribute__((__noinline__))
>  #define __aligned(x)           __attribute__((__aligned__(x)))
> @@ -59,16 +102,44 @@ typedef uint16_t           u16;
>  typedef uint32_t               u32;
>  typedef uint64_t               u64;
>
> -#define module_init(func)      static int (*do_init)(void) = func
> -#define module_exit(func)      static void (*do_exit)(void) = func
> -#define MODULE_LICENSE(str)    int main(void) {                \
> -                                       int rc;                 \
> -                                       /* License: str */      \
> -                                       rc = do_init();         \
> -                                       if (rc == 0)            \
> -                                               do_exit();      \
> -                                       return rc;              \
> -                               }
> +#define MODULE_LICENSE(str)    /* */
> +
> +int do_kunit_test_suite(struct kunit_suite *suite)
> +{
> +       const struct kunit_case *test_case;
> +       int rc = 0;
> +
> +       for (test_case = suite->test_cases; test_case->run_case; test_case++) {
> +               struct kunit test = { };
> +
> +               test_case->run_case(&test);
> +               switch (test.status) {
> +               default:
> +               case TEST_FAIL:
> +                       fprintf(stderr, "FAIL: %s%s%s", test_case->name,
> +                               test.msg ? ": " : "",
> +                               test.msg ?: "\n");
> +                       rc = 1;
> +                       break;
> +               case TEST_SKIP:
> +                       fprintf(stdout, "XFAIL: %s%s%s", test_case->name,
> +                               test.msg ? ": " : "",
> +                               test.msg ?: "\n");
> +                       break;
> +               case TEST_PASS:
> +                       fprintf(stdout, "ok: %s\n", test_case->name);
> +                       break;
> +               }
> +               if (test.msg)
> +                       free(test.msg);
> +       }
> +       return rc;
> +}
> +
> +#define kunit_test_suite(suite)                                        \
> +int main(void) {                                               \
> +       return do_kunit_test_suite(&(suite));                   \
> +}

very optional:
emulating kunit_test_suites() might be more future-proof here, if we
ever want to setup more suites.
There's little reason to do so right now given the lack of init and
exit support.

Like the stuff above, it wouldn't be hard to do, but I can see it not
being worth the extra code, i.e.

#define kunit_test_suites(suites...) \
  int main(void) {
     static struct kunit_suite *suites =[] = { __VA_ARGS__ };
     int i, ret = 0;
     for (i = 0; i < ARRAY_SIZE(suites); ++i)
       ret += do_kunit_test_suite(suites[i]);
     return ret;
   }