Re: [PATCH v3 4/4] rv/reactors: add KUnit tests for reactor registration and dispatch

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