| Message ID | 20210914210348.717392-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b7cbaef303c7b9f26c647bcba72da04dd35396c4 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [v5,1/2] kunit: drop assumption in kunit-log-test about current suite | expand |
On Wed, Sep 15, 2021 at 5:03 AM Daniel Latypov <dlatypov@google.com> wrote: > > This test assumes that the declared kunit_suite object is the exact one > which is being executed, which KUnit will not guarantee [1]. > > Specifically, `suite->log` is not initialized until a suite object is > executed. So if KUnit makes a copy of the suite and runs that instead, > this test dereferences an invalid pointer and (hopefully) segfaults. > > N.B. since we no longer assume this, we can no longer verify that > `suite->log` is *not* allocated during normal execution. > > An alternative to this patch that would allow us to test that would > require exposing an API for the current test to get its current suite. > Exposing that for one internal kunit test seems like overkill, and > grants users more footguns (e.g. reusing a test case in multiple suites > and changing behavior based on the suite name, dynamically modifying the > setup/cleanup funcs, storing/reading stuff out of the suite->log, etc.). > > [1] In a subsequent patch, KUnit will allow running subsets of test > cases within a suite by making a copy of the suite w/ the filtered test > list. But there are other reasons KUnit might execute a copy, e.g. if it > ever wants to support parallel execution of different suites, recovering > from errors and restarting suites > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com> > --- Thanks for fixing this. I do think that using "fake" tests/suites like this in more cases will unlock testing other parts of KUnit as well. Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > lib/kunit/kunit-test.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c > index d69efcbed624..555601d17f79 100644 > --- a/lib/kunit/kunit-test.c > +++ b/lib/kunit/kunit-test.c > @@ -415,12 +415,15 @@ static struct kunit_suite kunit_log_test_suite = { > > static void kunit_log_test(struct kunit *test) > { > - struct kunit_suite *suite = &kunit_log_test_suite; > + struct kunit_suite suite; > + > + suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log); > > kunit_log(KERN_INFO, test, "put this in log."); > kunit_log(KERN_INFO, test, "this too."); > - kunit_log(KERN_INFO, suite, "add to suite log."); > - kunit_log(KERN_INFO, suite, "along with this."); > + kunit_log(KERN_INFO, &suite, "add to suite log."); > + kunit_log(KERN_INFO, &suite, "along with this."); > > #ifdef CONFIG_KUNIT_DEBUGFS > KUNIT_EXPECT_NOT_ERR_OR_NULL(test, > @@ -428,12 +431,11 @@ static void kunit_log_test(struct kunit *test) > KUNIT_EXPECT_NOT_ERR_OR_NULL(test, > strstr(test->log, "this too.")); > KUNIT_EXPECT_NOT_ERR_OR_NULL(test, > - strstr(suite->log, "add to suite log.")); > + strstr(suite.log, "add to suite log.")); > KUNIT_EXPECT_NOT_ERR_OR_NULL(test, > - strstr(suite->log, "along with this.")); > + strstr(suite.log, "along with this.")); > #else > KUNIT_EXPECT_PTR_EQ(test, test->log, (char *)NULL); > - KUNIT_EXPECT_PTR_EQ(test, suite->log, (char *)NULL); > #endif > } > > > base-commit: 316346243be6df12799c0b64b788e06bad97c30b > -- > 2.33.0.309.g3052b89438-goog >
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index d69efcbed624..555601d17f79 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -415,12 +415,15 @@ static struct kunit_suite kunit_log_test_suite = { static void kunit_log_test(struct kunit *test) { - struct kunit_suite *suite = &kunit_log_test_suite; + struct kunit_suite suite; + + suite.log = kunit_kzalloc(test, KUNIT_LOG_SIZE, GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, suite.log); kunit_log(KERN_INFO, test, "put this in log."); kunit_log(KERN_INFO, test, "this too."); - kunit_log(KERN_INFO, suite, "add to suite log."); - kunit_log(KERN_INFO, suite, "along with this."); + kunit_log(KERN_INFO, &suite, "add to suite log."); + kunit_log(KERN_INFO, &suite, "along with this."); #ifdef CONFIG_KUNIT_DEBUGFS KUNIT_EXPECT_NOT_ERR_OR_NULL(test, @@ -428,12 +431,11 @@ static void kunit_log_test(struct kunit *test) KUNIT_EXPECT_NOT_ERR_OR_NULL(test, strstr(test->log, "this too.")); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, - strstr(suite->log, "add to suite log.")); + strstr(suite.log, "add to suite log.")); KUNIT_EXPECT_NOT_ERR_OR_NULL(test, - strstr(suite->log, "along with this.")); + strstr(suite.log, "along with this.")); #else KUNIT_EXPECT_PTR_EQ(test, test->log, (char *)NULL); - KUNIT_EXPECT_PTR_EQ(test, suite->log, (char *)NULL); #endif }