Message-ID: <CABVgOSn+F3WOA5hR+H5x_fW=jRTGATmO+nycZ7R7mXN8X7qNvw@mail.gmail.com>
Date:   Fri, 27 Jan 2023 14:21:15 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     Brendan Higgins <brendan.higgins@...ux.dev>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Rae Moar <rmoar@...gle.com>,
        Sadiya Kazi <sadiyakazi@...gle.com>,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2] kunit: Add "hooks" to call into KUnit when it's
 built as a module

On Fri, 27 Jan 2023 at 13:38, 'Daniel Latypov' via KUnit Development
<kunit-dev@...glegroups.com> wrote:
>
> On Tue, Jan 24, 2023 at 12:04 AM David Gow <davidgow@...gle.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 didn't think of that: it'd definitely fix the need to have a macro
for resetting the pointers to NULL, as well as the definitions.

We'd still have the repetition of the function names in the
kunit_set_hooks function and the table definition. (As well as needing
all of the implementation functions around.)

Still, I think there's value in putting this in a struct, even if we
also use the macro magic for other things. If we, for instance,
instead of setting individual members of struct kunit_hook_table, we
re-initialised it in one go, that might give the compiler enough
context to warn about uninitialised values if we missed one.

The only downside of the struct is that it's slightly uglier if people
want to call the hook function pointer directly. This is not as likely
at the moment, as it could be NULL, but it'd be possible to extend the
macro to generate a stub implementation which just returned nothing.
(Still, it'd be equally possible to autogenerate a wrapper function
which checks kunit_running, so this isn't a dealbreaker.)

> 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

This is actually the bit which pushed me over the line and made me
write the macro-based version: if the hook implementations are not all
in test.c (and the static stub ones are in static_stub.c), we'd have
to declare the hook implementation function somewhere, either
introducing a new hooks-impl.h or having a bunch of function
declarations in test.c.

My thought was, if we were going to need an extra header with more
definitions, we might as well just have one, which would also stop us
from worrying about missing an assignment in one place or the other.

> 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));
> }
>

I'll give moving this to a struct a go, it should be an improvement
even if I still use the macros to generate the struct.

The real advantage of the macro is only having to add the new hook in
two or three places:
- The hooks-table.h entry
- The _impl function (which can be in any file)
- (Optionally) a wrapper so that people don't need to check for NULL /
kunit_running themselves.

Without it, they'll have to:
- Add an entry to the struct kunit_hooks_table
- Write the implementation function.
- Add a declaration for the _impl function in test.c, if the function
isn't defined there.
- Add an entry to kunit_set_hooks()
- (Optionally) a wrapper, etc.

That's potentially twice as many things to get right. Still, it's a
lot better with the struct than doing each function individually, so
it's a closer tradeoff.
Personally, I still feel the macro-based version will eventually be
useful, but it's probably 50/50 whether it's worth it for just two or
three hooks.

Worst-case, we can do just the manual struct-based version, and
replace it with the macro one later.

Thanks,
-- David

> > 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@...gle.com>
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@...glegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAGS_qxq4vWvRJ89477S%2BrxHYLvnc2xN435GQ4%2BBvpLgqon8miw%40mail.gmail.com.

Powered by blists - more mailing lists