| Message ID | 20220502093625.GA23225@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7466886b400b1904ce30fa311904849e314a2cf4 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: take `kunit_assert` as `const` | expand |
On Mon, May 2, 2022 at 4:36 AM Miguel Ojeda <ojeda@kernel.org> wrote: > > The `kunit_do_failed_assertion` function passes its > `struct kunit_assert` argument to `kunit_fail`. This one, > in turn, calls its `format` field passing the assert again > as a `const` pointer. > > Therefore, the whole chain may be made `const`. > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Reviewed-by: Daniel Latypov <dlatypov@google.com> Thanks for this, the code definitely should have been this way from the start. I had wanted to make this change but mistakenly thought the format func took it via non-const for some reason. I must have misread it once and got it into my head that we were leaving the door open for mutable child structs (which sounds like a bad idea). > --- > include/kunit/test.h | 2 +- > lib/kunit/test.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 00b9ff7783ab..2eff4f1beb42 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -774,7 +774,7 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); > void kunit_do_failed_assertion(struct kunit *test, > const struct kunit_loc *loc, > enum kunit_assert_type type, > - struct kunit_assert *assert, > + const struct kunit_assert *assert, > const char *fmt, ...); > > #define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \ > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 3bca3bf5c15b..b84aed09a009 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -241,7 +241,7 @@ static void kunit_print_string_stream(struct kunit *test, > } > > static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, > - enum kunit_assert_type type, struct kunit_assert *assert, > + enum kunit_assert_type type, const struct kunit_assert *assert, > const struct va_format *message) > { > struct string_stream *stream; > @@ -281,7 +281,7 @@ static void __noreturn kunit_abort(struct kunit *test) > void kunit_do_failed_assertion(struct kunit *test, > const struct kunit_loc *loc, > enum kunit_assert_type type, > - struct kunit_assert *assert, > + const struct kunit_assert *assert, > const char *fmt, ...) > { > va_list args; > -- > 2.35.3 > > -- > 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/20220502093625.GA23225%40kernel.org.
Hi Daniel, On Mon, May 2, 2022 at 9:44 PM Daniel Latypov <dlatypov@google.com> wrote: > > Reviewed-by: Daniel Latypov <dlatypov@google.com> > > Thanks for this, the code definitely should have been this way from the start. > > I had wanted to make this change but mistakenly thought the format > func took it via non-const for some reason. > I must have misread it once and got it into my head that we were > leaving the door open for mutable child structs (which sounds like a > bad idea). Thanks for reviewing it so quickly! Yeah, I was unsure too if there was an external reason such as some future plan to use the mutability as you mention or maybe some out-of-tree user was relying on it already. But I thought it would be best to make it stricter until it is actually needed (if ever); or if there is an actual user for mutability, it should be documented/noted in-tree. It also simplifies a tiny bit a Rust-side call to `kunit_do_failed_assertion` that I am using within generated Rust documentation tests. Cheers, Miguel
On Wed, May 4, 2022 at 4:09 PM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Hi Daniel, > > On Mon, May 2, 2022 at 9:44 PM Daniel Latypov <dlatypov@google.com> wrote: > > > > Reviewed-by: Daniel Latypov <dlatypov@google.com> > > > > Thanks for this, the code definitely should have been this way from the start. > > > > I had wanted to make this change but mistakenly thought the format > > func took it via non-const for some reason. > > I must have misread it once and got it into my head that we were > > leaving the door open for mutable child structs (which sounds like a > > bad idea). > > Thanks for reviewing it so quickly! Yeah, I was unsure too if there > was an external reason such as some future plan to use the mutability > as you mention or maybe some out-of-tree user was relying on it > already. > > But I thought it would be best to make it stricter until it is > actually needed (if ever); or if there is an actual user for > mutability, it should be documented/noted in-tree. I definitely agree here -- I can't recall any particular plan that would require this to be non-const, and we can always change it back if we really need to. > It also simplifies a tiny bit a Rust-side call to > `kunit_do_failed_assertion` that I am using within generated Rust > documentation tests. Very exciting! I assume that's the PR here: https://github.com/Rust-for-Linux/linux/pull/757 Cheers, -- David
Hi David, On Wed, May 4, 2022 at 4:05 PM David Gow <davidgow@google.com> wrote: > > I definitely agree here -- I can't recall any particular plan that > would require this to be non-const, and we can always change it back > if we really need to. That is good to know, thanks! Out-of-tree users can always be a surprise... :) > Very exciting! I assume that's the PR here: > https://github.com/Rust-for-Linux/linux/pull/757 Indeed! I hope you like it -- we are taking the documentation tests in Rust (which are a very lightweight way of writing examples which double as tests) and generating KUnit test cases on the fly. For the moment it is just for the `kernel` crate, but the idea is to generalize it for modules etc. By the way, since you saw the PR... do you know if KUnit relies (or will rely) on "stack-dumping" functions like `longjmp`? Cheers, Miguel
On Thu, May 5, 2022 at 3:26 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Hi David, > > On Wed, May 4, 2022 at 4:05 PM David Gow <davidgow@google.com> wrote: > > > > I definitely agree here -- I can't recall any particular plan that > > would require this to be non-const, and we can always change it back > > if we really need to. > > That is good to know, thanks! Out-of-tree users can always be a surprise... :) > > > Very exciting! I assume that's the PR here: > > https://github.com/Rust-for-Linux/linux/pull/757 > > Indeed! I hope you like it -- we are taking the documentation tests in > Rust (which are a very lightweight way of writing examples which > double as tests) and generating KUnit test cases on the fly. For the > moment it is just for the `kernel` crate, but the idea is to > generalize it for modules etc. > > By the way, since you saw the PR... do you know if KUnit relies (or > will rely) on "stack-dumping" functions like `longjmp`? I don't think so -- though there's no fundamental individual tests couldn't use them if it made sense for them. KUnit spins off a new kthread per test, and uses kthread_complete_and_exit() to unwind when an assertion fails. See lib/kunit/try-catch.c for the actual implementation. The only really dodgy bit is the test timeout support, which attempts to stop a thread with kthread_stop(), and IIRC has some problems. Hope that helps! -- David
On Mon, May 02, 2022 at 11:36:25AM +0200, Miguel Ojeda wrote: > The `kunit_do_failed_assertion` function passes its > `struct kunit_assert` argument to `kunit_fail`. This one, > in turn, calls its `format` field passing the assert again > as a `const` pointer. > > Therefore, the whole chain may be made `const`. > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> Reviewed-by: Kees Cook <keescook@chromium.org>
diff --git a/include/kunit/test.h b/include/kunit/test.h index 00b9ff7783ab..2eff4f1beb42 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -774,7 +774,7 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...); void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, - struct kunit_assert *assert, + const struct kunit_assert *assert, const char *fmt, ...); #define KUNIT_ASSERTION(test, assert_type, pass, assert_class, INITIALIZER, fmt, ...) do { \ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 3bca3bf5c15b..b84aed09a009 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -241,7 +241,7 @@ static void kunit_print_string_stream(struct kunit *test, } static void kunit_fail(struct kunit *test, const struct kunit_loc *loc, - enum kunit_assert_type type, struct kunit_assert *assert, + enum kunit_assert_type type, const struct kunit_assert *assert, const struct va_format *message) { struct string_stream *stream; @@ -281,7 +281,7 @@ static void __noreturn kunit_abort(struct kunit *test) void kunit_do_failed_assertion(struct kunit *test, const struct kunit_loc *loc, enum kunit_assert_type type, - struct kunit_assert *assert, + const struct kunit_assert *assert, const char *fmt, ...) { va_list args;
The `kunit_do_failed_assertion` function passes its `struct kunit_assert` argument to `kunit_fail`. This one, in turn, calls its `format` field passing the assert again as a `const` pointer. Therefore, the whole chain may be made `const`. Signed-off-by: Miguel Ojeda <ojeda@kernel.org> --- include/kunit/test.h | 2 +- lib/kunit/test.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)