On Mon, Oct 7, 2019 at 6:03 PM Kees Cook keescook@chromium.org wrote: (...)
+static void list_init_test(struct kunit *test) +{
/* Test the different ways of initialising a list. */struct list_head list1 = LIST_HEAD_INIT(list1);struct list_head list2;LIST_HEAD(list3);INIT_LIST_HEAD(&list2);Can you also add different storage locations and initial contents tests? For example:
struct list_head *list4; struct list_head *list5; list4 = kzalloc(sizeof(*list4)); INIT_LIST_HEAD(list4); list5 = kmalloc(sizeof(*list5)); memset(list4, 0xff, sizeof(*list5)); INIT_LIST_HEAD(list5); KUNIT_EXPECT_TRUE(test, list_empty_careful(&list4)); KUNIT_EXPECT_TRUE(test, list_empty_careful(&list5)); kfree(list4); kfree(list5);Then we know it's not an accident that INIT_LIST_HEAD() works when both all-zeros and all-0xFF initial contents are there. :)
Will do.
+static void list_entry_test(struct kunit *test) +{
struct list_test_struct test_struct;I guess this is not a missing initialization here because this is just testing the container_of() wrapper macro?
Yeah: we shouldn't be doing any memory accesses here, just the pointer manipulation, so it shouldn't matter. I can add a comment pointing this out, or just initialise it anyway.
KUNIT_EXPECT_PTR_EQ(test, &test_struct, list_entry(&(test_struct.list), struct list_test_struct, list));+}
+static void list_first_entry_test(struct kunit *test) +{
struct list_test_struct test_struct1, test_struct2;static LIST_HEAD(list);This is this static?
Oops, this doesn't need to be static. I'll fix this (and the others) for the next version.
+static void list_for_each_test(struct kunit *test) +{
struct list_head entries[3], *cur;LIST_HEAD(list);int i = 0;list_add_tail(&entries[0], &list);list_add_tail(&entries[1], &list);list_add_tail(&entries[2], &list);list_for_each(cur, &list) {KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]);i++;}KUNIT_EXPECT_EQ(test, i, 3);+}
+static void list_for_each_prev_test(struct kunit *test) +{
struct list_head entries[3], *cur;LIST_HEAD(list);int i = 2;list_add_tail(&entries[0], &list);list_add_tail(&entries[1], &list);list_add_tail(&entries[2], &list);list_for_each_prev(cur, &list) {KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]);i--;}KUNIT_EXPECT_EQ(test, i, -1);Both of these tests test that the list contained the correct number of entries, not that anything about ordering was established. I would load values into these with the list_test_struct and test that the counting matches the expected ordering.
These tests do check the order of the entries (the order of the list should match the order of the entries array, and KUNIT_EXPECT_PTR_EQ() is verifying that the entries[i] is correct). It would be possible to actually use list_test_struct like with the list_for_each_entry tests, but since list_for_each just returns the list_head, it didn't seem useful, so long as the list_head pointers match.
+}
+static void list_for_each_safe_test(struct kunit *test) +{
struct list_head entries[3], *cur, *n;LIST_HEAD(list);int i = 0;list_add_tail(&entries[0], &list);list_add_tail(&entries[1], &list);list_add_tail(&entries[2], &list);list_for_each_safe(cur, n, &list) {KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]);list_del(&entries[i]);i++;}KUNIT_EXPECT_EQ(test, i, 3);I would expect an is_empty() test here too.
Will do.
+}
+static void list_for_each_prev_safe_test(struct kunit *test) +{
struct list_head entries[3], *cur, *n;LIST_HEAD(list);int i = 2;list_add_tail(&entries[0], &list);list_add_tail(&entries[1], &list);list_add_tail(&entries[2], &list);list_for_each_prev_safe(cur, n, &list) {KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]);list_del(&entries[i]);i--;}Same thing here.
Will do.
+static void list_for_each_entry_test(struct kunit *test) +{
struct list_test_struct entries[5], *cur;static LIST_HEAD(list);int i = 0;for (i = 0; i < 5; ++i) {entries[i].data = i;list_add_tail(&entries[i].list, &list);}i = 0;list_for_each_entry(cur, &list, list) {KUNIT_EXPECT_EQ(test, cur->data, i);i++;}+}
+static void list_for_each_entry_reverse_test(struct kunit *test) +{
struct list_test_struct entries[5], *cur;static LIST_HEAD(list);int i = 0;for (i = 0; i < 5; ++i) {entries[i].data = i;list_add_tail(&entries[i].list, &list);}i = 4;list_for_each_entry_reverse(cur, &list, list) {KUNIT_EXPECT_EQ(test, cur->data, i);i--;}Oh! Here is the data test. Why keep these separate? You could combine the counting tests with these?
One thing to consider adding is a short comment above each test to clarify the test intention. While these are relatively simple tests, it could clarify things like "only testing counts here" or "similar to test_foo(), this adds in ordering verification", etc.
The idea here was to have a separate test for each function/macro being tested. This hopefully should make it easier to narrow down where test failures are. In this case, the tests using list_test_struct are here because list_for_each_entry() does the implicit container_of(), so testing it properly requires the test struct. As above, the list_for_each() tests do actually check the order, so it's probably worth adding a check for the count to these tests, too.
There are definitely a few places where extra comments make sense. In general, for these tests at least, the purpose of each test is to test the function/macro it's named after, ideally reasonably thoroughly. My feeling is that, given that, it's more useful to call out explicitly if something obvious isn't tested (such as the list_empty_careful_test(), perhaps, which doesn't verify concurrent access from multiple threads).
Cheers, -- David