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(-)
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;
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.....
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
linux-kselftest-mirror@lists.linaro.org