On Tue, Aug 23, 2022 at 10:25 PM 'Joe Fradley' via KUnit Development
<kunit-dev@xxxxxxxxxxxxxxxx> wrote:
>
> This patch adds the kunit.enable module parameter that will need to be
> set to true in addition to KUNIT being enabled for KUnit tests to run.
> The default value is true giving backwards compatibility. However, for
> the production+testing use case the new config option
> KUNIT_DEFAULT_ENABLED can be set to N requiring the tester to opt-in
> by passing kunit.enable=1 to the kernel.
>
> Signed-off-by: Joe Fradley <joefradley@xxxxxxxxxx>
> ---
Thanks very much. This looks good to me, and works on my machine.
I've put a few comments/ideas below, but none of them feel necessary to me.
Regardless, this is
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
Cheers,
-- David
> Changes since v1:
> - Created a function to get kunit enable state
> - Check kunit enable state in kunit_run_all_tests() in executor.c
> - Load test module even if KUnit is disabled but still don't execute
> tests
> - Simplified kunit disable message and kunit.enable parameter
> description
> - Flipped around logic of new config to be KUNIT_DEFAULT_ENABLED
> - kunit_tool.py now passes kunit.enable=1 to kernel
>
> .../admin-guide/kernel-parameters.txt | 6 +++++
> include/kunit/test.h | 2 ++
> lib/kunit/Kconfig | 11 +++++++++
> lib/kunit/executor.c | 4 ++++
> lib/kunit/test.c | 24 +++++++++++++++++++
> tools/testing/kunit/kunit_kernel.py | 1 +
> 6 files changed, 48 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index adfda56b2691..7aa3abd7f1c5 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2432,6 +2432,12 @@
> 0: force disabled
> 1: force enabled
>
> + kunit.enable= [KUNIT] Enable executing KUnit tests. Requires
> + CONFIG_KUNIT to be set to be fully enabled. The
> + default value can be overridden via
> + KUNIT_DEFAULT_ENABLED.
> + Default is 1 (enabled)
> +
> kvm.ignore_msrs=[KVM] Ignore guest accesses to unhandled MSRs.
> Default is 0 (don't ignore, but inject #GP)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index c958855681cc..ee6bf4ecbd89 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -228,6 +228,8 @@ static inline void kunit_set_failure(struct kunit *test)
> WRITE_ONCE(test->status, KUNIT_FAILURE);
> }
>
> +bool kunit_enabled(void);
> +
This probably isn't strictly necessary at this stage, given that it
just checks one variable. That being said, I don't think it hurts (and
personally, I quite like it), and find it more future-proof than
exposing the variable more widely anyway.
> void kunit_init_test(struct kunit *test, const char *name, char *log);
>
> int kunit_run_tests(struct kunit_suite *suite);
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 0b5dfb001bac..626719b95bad 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -59,4 +59,15 @@ config KUNIT_ALL_TESTS
>
> If unsure, say N.
>
> +config KUNIT_DEFAULT_ENABLED
> + bool "Default value of kunit.enable"
> + default y
> + help
> + Sets the default value of kunit.enable. If set to N then KUnit
> + tests will not execute unless kunit.enable=1 is passed to the
> + kernel command line.
> +
> + In most cases this should be left as Y. Only if additional opt-in
> + behavior is needed should this be set to N.
> +
> endif # KUNIT
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 5e223327196a..9bbc422c284b 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -190,6 +190,10 @@ int kunit_run_all_tests(void)
> {
> struct suite_set suite_set = {__kunit_suites_start, __kunit_suites_end};
> int err = 0;
> + if (!kunit_enabled()) {
> + pr_info("kunit: disabled\n");
> + goto out;
> + }
>
> if (filter_glob_param) {
> suite_set = kunit_filter_suites(&suite_set, filter_glob_param, &err);
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index b73d5bb5c473..1e54373309a4 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -54,6 +54,17 @@ void __kunit_fail_current_test(const char *file, int line, const char *fmt, ...)
> EXPORT_SYMBOL_GPL(__kunit_fail_current_test);
> #endif
>
> +/*
> + * Enable KUnit tests to run.
> + */
> +#ifdef CONFIG_KUNIT_DEFAULT_ENABLED
> +static bool enable_param = true;
> +#else
> +static bool enable_param;
> +#endif
> +module_param_named(enable, enable_param, bool, 0);
> +MODULE_PARM_DESC(enable, "Enable KUnit tests");
> +
> /*
> * KUnit statistic mode:
> * 0 - disabled
> @@ -586,10 +597,20 @@ static void kunit_init_suite(struct kunit_suite *suite)
> suite->suite_init_err = 0;
> }
>
> +bool kunit_enabled(void)
> +{
> + return enable_param;
> +}
> +
> int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites)
> {
> unsigned int i;
>
> + if (!kunit_enabled() && num_suites > 0) {
> + pr_info("kunit: disabled\n");
_Maybe_ this could be pr_info_once(), if you were worried about spam
(if a whole bunch of test modules were loaded at once). That being
said, I prefer it as-is, as I don't think there are a lot of cases
where large number of kunit test modules are loaded on a system with
KUnit disable. And I'm liable to forget that KUnit is disabled if a
system has been running for a while (and maybe one test module was
loaded a boot), and end up wondering why my test isn't running.
So, I'm all for leaving this as-is, personally.
> + return 0;
> + }
> +
> for (i = 0; i < num_suites; i++) {
> kunit_init_suite(suites[i]);
> kunit_run_tests(suites[i]);
> @@ -607,6 +628,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites)
> {
> unsigned int i;
>
> + if (!kunit_enabled())
> + return;
> +
> for (i = 0; i < num_suites; i++)
> kunit_exit_suite(suites[i]);
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index f5c26ea89714..ef794da420d7 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -359,6 +359,7 @@ class LinuxSourceTree:
> args = []
> if filter_glob:
> args.append('kunit.filter_glob='+filter_glob)
> + args.append('kunit.enable=1')
>
> process = self._ops.start(args, build_dir)
> assert process.stdout is not None # tell mypy it's set
> --
> 2.37.1.595.g718a3a8f04-goog
>
> --
> 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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220823142456.3977086-2-joefradley%40google.com.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
