mbox series

[0/4] kunit: more assertion reworking

Message ID 20221001002638.2881842-1-dlatypov@google.com (mailing list archive)
Headers show
Series kunit: more assertion reworking | expand

Message

Daniel Latypov Oct. 1, 2022, 12:26 a.m. UTC
RFC: https://lore.kernel.org/linux-kselftest/20220525154442.1438081-1-dlatypov@google.com/
Changes since then: tweak commit messages, reformatting to make
  checkpatch.pl happy. Nothing substantial.
Why send this out again now: the initial Rust patchset no longer
  contains the Kunit changes, so hopefully both series can go into 6.1
  and later we can coordinate the update the kunit.rs wrapper.

This is a follow up to these three series:
https://lore.kernel.org/all/20220113165931.451305-1-dlatypov@google.com/
https://lore.kernel.org/all/20220118223506.1701553-1-dlatypov@google.com/
https://lore.kernel.org/all/20220125210011.3817742-1-dlatypov@google.com/

The two goals of those series were
a) reduce the size of struct kunit_assert and friends.
   (struct kunit_assert went from 48 => 8 bytes on UML.)
b) simplify the internal code, mostly by deleting macros

This series goes further
a) sizeof(struct kunit_assert) = 0 now
b) e.g. we delete another class of macros (KUNIT_INIT_*_ASSERT_STRUCT)

Note: this does change the function signature of
kunit_do_failed_assertion, so we'd need to update the rust wrapper in
https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs,
but hopefully it's just a simple change, e.g. maybe just like:
@@ -38,9 +38,7 @@
             });
             static CONDITION: &'static $crate::str::CStr =
$crate::c_str!(stringify!($cond));
             static ASSERTION: UnaryAssert =
UnaryAssert($crate::bindings::kunit_unary_assert {
-                assert: $crate::bindings::kunit_assert {
-                    format: Some($crate::bindings::kunit_unary_assert_format),
-                },
+                assert: $crate::bindings::kunit_assert {},
                 condition: CONDITION.as_char_ptr(),
                 expected_true: true,
             });
@@ -67,6 +65,7 @@
                     core::ptr::addr_of!(LOCATION.0),
                     $crate::bindings::kunit_assert_type_KUNIT_ASSERTION,
                     core::ptr::addr_of!(ASSERTION.0.assert),
+                    Some($crate::bindings::kunit_unary_assert_format),
                     core::ptr::null(),
                 );
             }


Daniel Latypov (4):
  kunit: remove format func from struct kunit_assert, get it to 0 bytes
  kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED
  kunit: eliminate KUNIT_INIT_*_ASSERT_STRUCT macros
  kunit: declare kunit_assert structs as const

 include/kunit/assert.h |  74 ++----------------------
 include/kunit/test.h   | 127 +++++++++++++++++++++++------------------
 lib/kunit/test.c       |   7 ++-
 3 files changed, 80 insertions(+), 128 deletions(-)


base-commit: 511cce163b75bc3933fa3de769a82bb7e8663f2b

Comments

Miguel Ojeda Oct. 1, 2022, 10:15 a.m. UTC | #1
On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Note: this does change the function signature of
> kunit_do_failed_assertion, so we'd need to update the rust wrapper in
> https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs,
> but hopefully it's just a simple change, e.g. maybe just like:

Yeah, should be simple. Thanks for pointing it out!

The series looks like a great cleanup on top of the stack reduction.

Cheers,
Miguel
Daniel Latypov Oct. 1, 2022, 6 p.m. UTC | #2
On Sat, Oct 1, 2022 at 3:15 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Oct 1, 2022 at 2:26 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Note: this does change the function signature of
> > kunit_do_failed_assertion, so we'd need to update the rust wrapper in
> > https://github.com/Rust-for-Linux/linux/blob/rust/rust/kernel/kunit.rs,
> > but hopefully it's just a simple change, e.g. maybe just like:
>
> Yeah, should be simple. Thanks for pointing it out!
>
> The series looks like a great cleanup on top of the stack reduction.

Thanks for taking a look at the rest of the series as well.

While I have you here, any thoughts on how to coordinate the change?
I made the breaking change patch#1 so it should be easier to pull out.

One option I was thinking was:
* wait till this lands in Shuah's tree
* I create a Github PR that contains patch#1 + a patch for kunit.rs

I was not clear on how the RfL Github pulls in upstream changes or how often.
But my assumption is patch#1 would fall away naturally if rebasing
onto 6.1 (and maybe we can squash the kunit.rs change).

Thanks,
Daniel
Miguel Ojeda Oct. 18, 2022, 11:20 p.m. UTC | #3
On Sat, Oct 1, 2022 at 8:00 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> While I have you here, any thoughts on how to coordinate the change?

My bad, I forgot to reply to this, sorry. I noticed it again when
merging 6.1-rc1 into our branch.

Normally I fix the issues when I merge back from Linus. Since we
intend to keep the CI green on every PR, I added the fix for this in
the merge itself.

In any case, to clarify, the `rust` branch in GitHub is just where we
placed a lot of things that we wanted to eventually submit (and some
that should not, e.g. the GitHub CI files). We will be minimizing the
differences w.r.t. upstream in that branch by preparing proper patches
from scratch and submitting them through `rust-next` and other trees,
and eventually remove it (or reusing the name for the fixes branch
since that is the name other trees use, but we will see).

Cheers,
Miguel
Daniel Latypov Oct. 18, 2022, 11:26 p.m. UTC | #4
On Tue, Oct 18, 2022 at 4:20 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Sat, Oct 1, 2022 at 8:00 PM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > While I have you here, any thoughts on how to coordinate the change?
>
> My bad, I forgot to reply to this, sorry. I noticed it again when
> merging 6.1-rc1 into our branch.

No worries.
You've had a very busy couple of weeks, I imagine.

>
> Normally I fix the issues when I merge back from Linus. Since we
> intend to keep the CI green on every PR, I added the fix for this in
> the merge itself.

Thanks!


Daniel
Miguel Ojeda Oct. 18, 2022, 11:39 p.m. UTC | #5
On Wed, Oct 19, 2022 at 1:26 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> No worries.
> You've had a very busy couple of weeks, I imagine.

Just a bit :) Nevertheless, it was my intention to reply :(

I have linked this thread from the PR noting that you warned me about
the future conflict [1], thanks again!

[1] https://github.com/Rust-for-Linux/linux/pull/915#issuecomment-1283138279

Cheers,
Miguel