MIME-Version: 1.0
References: <20230124080350.2275652-1-davidgow@google.com>
In-Reply-To: <20230124080350.2275652-1-davidgow@google.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Thu, 26 Jan 2023 21:38:29 -0800
Message-ID: <CAGS_qxq4vWvRJ89477S+rxHYLvnc2xN435GQ4+BvpLgqon8miw@mail.gmail.com>
Subject: Re: [RFC PATCH v2] kunit: Add "hooks" to call into KUnit when it's
 built as a module
To:     David Gow <davidgow@google.com>
Cc:     Brendan Higgins <brendan.higgins@linux.dev>,
        Kees Cook <keescook@chromium.org>,
        Shuah Khan <skhan@linuxfoundation.org>,
        Rae Moar <rmoar@google.com>,
        Sadiya Kazi <sadiyakazi@google.com>,
        kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 24, 2023 at 12:04 AM David Gow <davidgow@google.com> wrote:
>
> KUnit has several macros and functions intended for use from non-test
> code. These hooks, currently the kunit_get_current_test() and
> kunit_fail_current_test() macros, didn't work when CONFIG_KUNIT=m.
>
> In order to support this case, the required functions and static data
> need to be available unconditionally, even when KUnit itself is not
> built-in. The new 'hooks.c' file is therefore always included, and has
> both the static key required for kunit_get_current_test(), and a
> function pointer to the real implementation of
> __kunit_fail_current_test(), which is populated when the KUnit module is
> loaded.
>
> A new header, kunit/hooks-table.h, contains a table of all hooks, and is
> repeatedly included with different definitions of the KUNIT_HOOK() in
> order to automatically generate the needed function pointer tables. When

Perhaps I'm overlooking something and this is a dumb question.

Is there a reason we can't go with a less-clever approach?
Like have a global struct?
We could memset it to 0 to clear it instead of defining a macro to set
individual variables to NULL?

i.e.

// hooks.h
extern struct kunit_hook_table {
        __printf(3, 4) void (*fail_current_test)(const char*, int,
const char*, ...);
} kunit_hooks;

//hooks.c
struct kunit_hook_table kunit_hooks;

// in test.c
// here all the functions should be in scope for us to use
static void kunit_set_hooks(void)
{
  kunit_hooks.fail_current_test = __kunit_fail_current_test;
  ...
}

 static int __init kunit_init(void)
 {
  ...
  kunit_set_hooks();
  ...
}

static void __exit kunit_exit(void)
{
  ...
  memset(&kunit_hooks, 0, sizeof(kunit_hooks));
}

> KUnit is disabled, or the module is not loaded, these function pointers
> are all NULL. This shouldn't be a problem, as they're all used behind
> wrappers which check kunit_running and/or that the pointer is non-NULL.
>
> This can then be extended for future features which require similar
> "hook" behaviour, such as static stubs:
> https://lore.kernel.org/all/20221208061841.2186447-1-davidgow@google.com/
>
> Signed-off-by: David Gow <davidgow@google.com>