Messages in this thread | | | From | Andrey Konovalov <> | Date | Tue, 12 Jan 2021 18:55:53 +0100 | Subject | Re: [PATCH 03/11] kasan: clean up comments in tests |
| |
On Tue, Jan 12, 2021 at 8:53 AM Alexander Potapenko <glider@google.com> wrote: > > On Tue, Jan 5, 2021 at 7:28 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > > > Clarify and update comments and info messages in KASAN tests. > > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > > Link: https://linux-review.googlesource.com/id/I6c816c51fa1e0eb7aa3dead6bda1f339d2af46c8 > > > void *kasan_ptr_result; > > int kasan_int_result; > Shouldn't these two variables be static, by the way?
No, then the compiler starts eliminating accesses.
> > @@ -39,14 +38,13 @@ static struct kunit_resource resource; > > static struct kunit_kasan_expectation fail_data; > > static bool multishot; > > > > +/* > > + * Temporarily enable multi-shot mode. Otherwise, KASAN would only report the > > + * first detected bug and panic the kernel if panic_on_warn is enabled. > > + */ > > YMMV, but I think this comment was at its place already.
It gets updated by one of the subsequent patches.
> > static int kasan_test_init(struct kunit *test) > > { > > - /* > > - * Temporarily enable multi-shot mode and set panic_on_warn=0. > > - * Otherwise, we'd only get a report for the first case. > > - */ > > multishot = kasan_save_enable_multi_shot(); > > Unrelated to this change, but have you considered storing > test-specific data in test->priv instead of globals?
I'd say that test->priv is for some per-test data that's used in the tests, and multishot is not a part of that.
> > if (!IS_ENABLED(CONFIG_SLUB)) { > > - kunit_info(test, "CONFIG_SLUB is not enabled."); > > + kunit_info(test, "skipping, CONFIG_SLUB required"); > > return; > > } > > You may want to introduce a macro that takes a config name and prints > the warning/returns if it's not enabled.
Good idea, will do in v2.
Thanks!
| |