linux-kernel - Re: [PATCH] kunit: Introduce autorun option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+GJov4Kfo5EwjXRe8vufhMetsYzHG7bJS6VERaE8+1rNL69Vg@mail.gmail.com>
Date: Tue, 22 Oct 2024 17:16:31 -0400
From: Rae Moar <rmoar@...gle.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
Cc: brendan.higgins@...ux.dev, davidgow@...gle.com, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kunit: Introduce autorun option

On Thu, Oct 17, 2024 at 5:34 PM Stanislav Kinsburskii
<skinsburskii@...ux.microsoft.com> wrote:
>
> The new option controls tests run on boot or module load. With the new
> debugfs "run" dentry allowing to run tests on demand, an ability to disable
> automatic tests run becomes a useful option in case of intrusive tests.
>
> The option is set to true by default to preserve the existent behavior. It
> can be overridden by either the corresponding module option or by the
> corresponding config build option.
>
> Signed-off-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>

Hello!

This patch looks good to me! The functionality seems to be working
well. I do have one comment below.

Thanks!
-Rae

> ---
>  include/kunit/test.h |    4 +++-
>  lib/kunit/Kconfig    |   12 ++++++++++++
>  lib/kunit/debugfs.c  |    2 +-
>  lib/kunit/executor.c |   18 +++++++++++++++++-
>  lib/kunit/test.c     |    6 ++++--
>  5 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 34b71e42fb10..58dbab60f853 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -312,6 +312,7 @@ static inline void kunit_set_failure(struct kunit *test)
>  }
>
>  bool kunit_enabled(void);
> +bool kunit_autorun(void);
>  const char *kunit_action(void);
>  const char *kunit_filter_glob(void);
>  char *kunit_filter(void);
> @@ -334,7 +335,8 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>                     int *err);
>  void kunit_free_suite_set(struct kunit_suite_set suite_set);
>
> -int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites);
> +int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites,
> +                            bool run_tests);
>
>  void __kunit_test_suites_exit(struct kunit_suite **suites, int num_suites);
>
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 34d7242d526d..a97897edd964 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -81,4 +81,16 @@ config KUNIT_DEFAULT_ENABLED
>           In most cases this should be left as Y. Only if additional opt-in
>           behavior is needed should this be set to N.
>
> +config KUNIT_AUTORUN_ENABLED
> +       bool "Default value of kunit.autorun"
> +       default y
> +       help
> +         Sets the default value of kunit.autorun. If set to N then KUnit
> +         tests will not run after initialization unless kunit.autorun=1 is
> +         passed to the kernel command line. The test can still be run manually
> +         via debugfs interface.
> +
> +         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/debugfs.c b/lib/kunit/debugfs.c
> index d548750a325a..9df064f40d98 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -145,7 +145,7 @@ static ssize_t debugfs_run(struct file *file,
>         struct inode *f_inode = file->f_inode;
>         struct kunit_suite *suite = (struct kunit_suite *) f_inode->i_private;
>
> -       __kunit_test_suites_init(&suite, 1);
> +       __kunit_test_suites_init(&suite, 1, true);
>
>         return count;
>  }
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 34b7b6833df3..340723571b0f 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -29,6 +29,22 @@ const char *kunit_action(void)
>         return action_param;
>  }
>
> +/*
> + * Run KUnit tests after initialization
> + */
> +#ifdef CONFIG_KUNIT_AUTORUN_ENABLED
> +static bool autorun_param = true;
> +#else
> +static bool autorun_param;
> +#endif
> +module_param_named(autorun, autorun_param, bool, 0);
> +MODULE_PARM_DESC(autorun, "Run KUnit tests after initialization");
> +
> +bool kunit_autorun(void)
> +{
> +       return autorun_param;
> +}
> +
>  static char *filter_glob_param;
>  static char *filter_param;
>  static char *filter_action_param;
> @@ -266,7 +282,7 @@ void kunit_exec_run_tests(struct kunit_suite_set *suite_set, bool builtin)
>                 pr_info("1..%zu\n", num_suites);

When using this feature, I still see some KTAP output that are printed
from this function (kunit_exec_run_tests). I think it would be great
if we could remove this output as to not clutter the kernel log.

At first, I was confused as to why we needed to call this function and
initialize the tests but I realized the debugfs suites need to be
created.

So instead, could we check for kunit_autorun() here instead as a
condition before printing the output?

>         }
>
> -       __kunit_test_suites_init(suite_set->start, num_suites);
> +       __kunit_test_suites_init(suite_set->start, num_suites, kunit_autorun());
>  }
>
>  void kunit_exec_list_tests(struct kunit_suite_set *suite_set, bool include_attr)
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 089c832e3cdb..146d1b48a096 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -708,7 +708,8 @@ bool kunit_enabled(void)
>         return enable_param;
>  }
>
> -int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites)
> +int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_suites,
> +                            bool run_tests)
>  {
>         unsigned int i;
>
> @@ -731,7 +732,8 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites, int num_
>
>         for (i = 0; i < num_suites; i++) {
>                 kunit_init_suite(suites[i]);
> -               kunit_run_tests(suites[i]);
> +               if (run_tests)
> +                       kunit_run_tests(suites[i]);
>         }
>
>         static_branch_dec(&kunit_running);
>
>
> --
> 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/172920085854.4578.9203147717033046574.stgit%40skinsburskii-cloud-desktop.internal.cloudapp.net.