Re: [PATCH v2 0/4] kselftest: add fixture parameters

From: Kees Cook
Date: Sat Mar 14 2020 - 22:48:18 EST


On Fri, Mar 13, 2020 at 05:54:57PM -0700, Jakub Kicinski wrote:
> Note that we loose a little bit of type safety
> without passing parameters as an explicit argument.
> If user puts the name of the wrong fixture as argument
> to CURRENT_FIXTURE() it will happily cast the type.

This got me to take a much closer look at things. I really didn't like
needing to repeat the fixture name in CURRENT_FIXTURE() calls, and then
started coming to all the same conclusions you did in your v1, that I
just didn't quite see yet in my first review. :P

Apologies for my wishy-washy-ness on this, but here's me talking myself
out of my earlier criticisms:

- "I want tests to be run in declaration order" In v1, this is actually
mostly retained: they're still in declaration order, but they're
grouped by fixture (which are run in declaration order). That, I think,
is totally fine. Someone writing code that interleaves between fixtures
is madness, and having the report retain that ordering seems awful. I
had thought the declaration ordering was entirely removed, but I see on
closer inspection that's not true.

- "I'd like everything attached to _metadata" This results in the
type unsafety you call out here. And I stared at your v2 trying to
find a way around it, but to get the type attached, it has to be
part of the __TEST_F_IMPL() glue, and that means passing it along
side "self", which means plumbing it as a function argument
everywhere.

So, again, sorry for asking to iterate on v1 instead of v2, though the
v2 _really_ helped me see the problems better. ;)

Something I'd like for v3: instead of "parameters" can we call it
"instances"? It provides a way to run separate instances of the same
fixtures. Those instances have parameters (i.e. struct fields), so I'd
prefer the "instance" naming.

Also a change in reporting:

struct __fixture_params_metadata no_param = { .name = "", };

Let's make ".name = NULL" here, and then we can detect instantiation:

printf("[ RUN ] %s%s%s.%s\n", f->name, p->name ? "." : "",
p->name ?: "", t->name);

That'll give us single-instance fixtures an unchanged name:

fixture.test1
fixture.test2

and instanced fixtures will be:

fixture.wayA.test1
fixture.wayA.test2
fixture.wayB.test1
fixture.wayB.test2


And finally, since we're in the land of endless macros, I think it
could be possible to make a macro to generate the __register_foo()
routine bodies. By the end of the series there are three nearly identical
functions in the harness for __register_test(), __register_fixture(), and
__register_fixture_instance(). Something like this as an earlier patch to
refactor the __register_test() that can be used by the latter two in their
patches (and counting will likely need to be refactored earlier too):

#define __LIST_APPEND(head, item) \
{ \
/* Circular linked list where only prev is circular. */ \
if (head == NULL) { \
head = item; \
item->next = NULL; \
item->prev = item; \
return; \
} \
if (__constructor_order == _CONSTRUCTOR_ORDER_FORWARD) {\
item->next = NULL; \
item->prev = head->prev; \
item->prev->next = item; \
head->prev = item; \
} else { \
p->next = head; \
p->next->prev = item; \
p->prev = item; \
head = item; \
} \
}

Which should let it be used, ultimately, as:

static inline void __register_test(struct __test_metadata *t)
__LIST_APPEND(__test_list, t)

static inline void __register_fixture(struct __fixture_metadata *f)
__LIST_APPEND(__fixture_list, f)

static inline void
__register_fixture_instance(struct __fixture_metadata *f,
struct __fixture_instance_metadata *p)
__LIST_APPEND(f->instances, p)


Thanks for working on this!

-Kees

--
Kees Cook