On Fri, 27 Jan 2023 at 13:38, 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
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 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@google.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@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/CAGS_qxq4vWvRJ89477S%2BrxHYLvnc2....