On Mon, 2026-08-17 at 12:52 +0200, Nam Cao wrote:
> [email protected] writes:
> > +/*
> > + * Use a fixed-size array so sizeof() gives the exact byte count at
> > + * compile time.
> > + */
>
> There is no need for a comment. This is obvious from the code itself.
>
> > +static const char long_reactor_name[] = "kunit_reactor_name_too_long_xxx_";
> > +_Static_assert(sizeof(long_reactor_name) - 1 >= MAX_RV_REACTOR_NAME_SIZE,
> > + "long_reactor_name must be at least MAX_RV_REACTOR_NAME_SIZE
> > chars");
> > +
> > +static void test_name_too_long(struct kunit *test)
> > +{
> > + static struct rv_reactor long_reactor = {
> > + .name = long_reactor_name,
> > + };
> > +
> > + KUNIT_EXPECT_EQ(test, rv_register_reactor(&long_reactor), -EINVAL);
> > +}
> > +
> > +static struct kunit_case rv_reactor_registration_cases[] = {
> > + KUNIT_CASE(test_double_register),
> > + KUNIT_CASE(test_name_too_long),
> > + {}
> > +};
>
> I am not sure about the usefulness of these test cases.
> rv_register_reactor() is not an user API that we have to prepare
> for corner case usage. We can expect its users to be sane since the
> users are us.
>
> But well, if Gabriele wants to keep them..
>
That's right, it can be a bit of an overkill, but I'm thinking one day reactors
could come from (out-of-tree) kernel modules, it wouldn't hurt to have this path
tested, especially if it doesn't require much effort.
> > +
> > +static struct kunit_suite rv_reactor_registration_suite = {
> > + .name = "rv_reactor_registration",
> > + .test_cases = rv_reactor_registration_cases,
> > +};
> > +
> > +static atomic_t react_call_count;
>
> Do we really need atomic_t? Does int work?
Missed that, yes, it should work just fine since KUnit runs in a single thread.
>
> > +
> > +__printf(1, 0) static void mock_react(const char *msg, va_list args)
> > +{
> > + atomic_inc(&react_call_count);
> > + /*
> > + * Hold the CPU for 5 ms so a timer interrupt is likely to fire
> > + * inside rv_react()'s lockdep context, exercising the LD_WAIT_SPIN
> > + * constraint. mdelay() is a calibrated busy-wait with no
> > scheduler
> > + * interaction.
> > + */
>
> The comment above mdelay()'s definition already explains what it does.
>
> > + mdelay(5);
> > +}
> > +
> > +static void test_react_no_callback(struct kunit *test)
> > +{
> > + struct rv_monitor monitor = {
> > + .name = "kunit_null_react",
> > + };
> > +
> > + atomic_set(&react_call_count, 0);
> > + rv_react(&monitor, "no callback");
> > +
> > + /*
> > + * The only possible failure in this test case is a kernel panic.
> > + * NULL react guard: callback must NOT have been invoked
> > + */
>
> Obvious comment.
>
> > + KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 0);
> > +}
> > +
> > +static void test_react_callback_invoked(struct kunit *test)
> > +{
> > + struct rv_monitor monitor = {
> > + .name = "kunit_dispatch_monitor",
> > + .react = mock_react,
> > + };
> > +
> > + atomic_set(&react_call_count, 0);
> > + rv_react(&monitor, "callback invocation test");
> > + KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 1);
> > +}
>
> So the test calls rv_react(), and validates that the reactor is called?
> Honestly I am not sure how useful that is. Especially since Gabriele
> already made the selftest which validates that the reactor is invoked.
>From what I understood, the value in this test suite isn't really about the
assertions (especially in test_react_no_callback). It triggers cases that should
not produce a splat (panic/lockdep) and a pass should consider that too.
This makes them not quite pure KUnit, but again, I think it doesn't hurt to have
this kind of test since KUnit output is in dmesg anyway, so a splat would hardly
slip.
What do you think?
Gabriele
