[PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h
Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled.
Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry).
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Brendan Higgins brendanhiggins@google.com Tested-by: Brendan Higgins brendanhiggins@google.com ---
This revision addresses Brendan's comments in https://lore.kernel.org/linux-kselftest/20191023220248.GA55483@google.com/
Specifically: - Brendan's Reviewed-by/Tested-by being included in the description. - A couple of trailing tabs in Kconfig.debug & list-test.c - Reformatting of previously >80 character lines.
Earlier versions of this patchset can be found:
v5: https://lore.kernel.org/linux-kselftest/20191022221322.122788-1-davidgow@goo... v4: https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-davidgow@goog... v3: https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-davidgow@goog... v2: https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-davidgow@goog... v1: https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-davidgow@goog...
Thanks, -- David
MAINTAINERS | 7 + lib/Kconfig.debug | 18 ++ lib/Makefile | 3 + lib/list-test.c | 746 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 774 insertions(+) create mode 100644 lib/list-test.c
diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..f3d0c6e42b97 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,13 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c
+LIST KUNIT TEST +M: David Gow davidgow@google.com +L: linux-kselftest@vger.kernel.org +L: kunit-dev@googlegroups.com +S: Maintained +F: lib/list-test.c + LIVE PATCHING M: Josh Poimboeuf jpoimboe@redhat.com M: Jiri Kosina jikos@kernel.org diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..6c1be6181e38 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST
If unsure, say N.
+config LIST_KUNIT_TEST + bool "KUnit Test for Kernel Linked-list structures" + depends on KUNIT + help + This builds the linked list KUnit test suite. + It tests that the API and basic functionality of the list_head type + and associated macros. + + KUnit tests run during boot and output the results to the debug log + in TAP format (http://testanything.org/). Only useful for kernel devs + running the KUnit test harness, and not intended for inclusion into a + production build. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index bba1fd5485f7..890e581d00c4 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o + +# KUnit tests +obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o diff --git a/lib/list-test.c b/lib/list-test.c new file mode 100644 index 000000000000..363c600491c3 --- /dev/null +++ b/lib/list-test.c @@ -0,0 +1,746 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for the Kernel Linked-list structures. + * + * Copyright (C) 2019, Google LLC. + * Author: David Gow davidgow@google.com + */ +#include <kunit/test.h> + +#include <linux/list.h> + +struct list_test_struct { + int data; + struct list_head list; +}; + +static void list_test_list_init(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); + struct list_head *list4; + struct list_head *list5; + + INIT_LIST_HEAD(&list2); + + list4 = kzalloc(sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL); + INIT_LIST_HEAD(list4); + + list5 = kmalloc(sizeof(*list5), GFP_KERNEL | __GFP_NOFAIL); + memset(list5, 0xFF, sizeof(*list5)); + INIT_LIST_HEAD(list5); + + /* list_empty_careful() checks both next and prev. */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list3)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list4)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(list5)); + + kfree(list4); + kfree(list5); +} + +static void list_test_list_add(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add(&a, &list); + list_add(&b, &list); + + /* should be [list] -> b -> a */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, b.next, &a); +} + +static void list_test_list_add_tail(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* should be [list] -> a -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a); + KUNIT_EXPECT_PTR_EQ(test, a.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, a.next, &b); +} + +static void list_test_list_del(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a -> b */ + list_del(&a); + + /* now: [list] -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); +} + +static void list_test_list_replace(struct kunit *test) +{ + struct list_head a_old, a_new, b; + LIST_HEAD(list); + + list_add_tail(&a_old, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a_old -> b */ + list_replace(&a_old, &a_new); + + /* now: [list] -> a_new -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a_new); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &a_new); +} + +static void list_test_list_replace_init(struct kunit *test) +{ + struct list_head a_old, a_new, b; + LIST_HEAD(list); + + list_add_tail(&a_old, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a_old -> b */ + list_replace_init(&a_old, &a_new); + + /* now: [list] -> a_new -> b */ + KUNIT_EXPECT_PTR_EQ(test, list.next, &a_new); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &a_new); + + /* check a_old is empty (initialized) */ + KUNIT_EXPECT_TRUE(test, list_empty_careful(&a_old)); +} + +static void list_test_list_swap(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a -> b */ + list_swap(&a, &b); + + /* after: [list] -> b -> a */ + KUNIT_EXPECT_PTR_EQ(test, &b, list.next); + KUNIT_EXPECT_PTR_EQ(test, &a, list.prev); + + KUNIT_EXPECT_PTR_EQ(test, &a, b.next); + KUNIT_EXPECT_PTR_EQ(test, &list, b.prev); + + KUNIT_EXPECT_PTR_EQ(test, &list, a.next); + KUNIT_EXPECT_PTR_EQ(test, &b, a.prev); +} + +static void list_test_list_del_init(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a -> b */ + list_del_init(&a); + /* after: [list] -> b, a initialised */ + + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&a)); +} + +static void list_test_list_move(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list1); + LIST_HEAD(list2); + + list_add_tail(&a, &list1); + list_add_tail(&b, &list2); + + /* before: [list1] -> a, [list2] -> b */ + list_move(&a, &list2); + /* after: [list1] empty, [list2] -> a -> b */ + + KUNIT_EXPECT_TRUE(test, list_empty(&list1)); + + KUNIT_EXPECT_PTR_EQ(test, &a, list2.next); + KUNIT_EXPECT_PTR_EQ(test, &b, a.next); +} + +static void list_test_list_move_tail(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list1); + LIST_HEAD(list2); + + list_add_tail(&a, &list1); + list_add_tail(&b, &list2); + + /* before: [list1] -> a, [list2] -> b */ + list_move_tail(&a, &list2); + /* after: [list1] empty, [list2] -> b -> a */ + + KUNIT_EXPECT_TRUE(test, list_empty(&list1)); + + KUNIT_EXPECT_PTR_EQ(test, &b, list2.next); + KUNIT_EXPECT_PTR_EQ(test, &a, b.next); +} + +static void list_test_list_bulk_move_tail(struct kunit *test) +{ + struct list_head a, b, c, d, x, y; + struct list_head *list1_values[] = { &x, &b, &c, &y }; + struct list_head *list2_values[] = { &a, &d }; + struct list_head *ptr; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&x, &list1); + list_add_tail(&y, &list1); + + list_add_tail(&a, &list2); + list_add_tail(&b, &list2); + list_add_tail(&c, &list2); + list_add_tail(&d, &list2); + + /* before: [list1] -> x -> y, [list2] -> a -> b -> c -> d */ + list_bulk_move_tail(&y, &b, &c); + /* after: [list1] -> x -> b -> c -> y, [list2] -> a -> d */ + + list_for_each(ptr, &list1) { + KUNIT_EXPECT_PTR_EQ(test, ptr, list1_values[i]); + i++; + } + KUNIT_EXPECT_EQ(test, i, 4); + i = 0; + list_for_each(ptr, &list2) { + KUNIT_EXPECT_PTR_EQ(test, ptr, list2_values[i]); + i++; + } + KUNIT_EXPECT_EQ(test, i, 2); +} + +static void list_test_list_is_first(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + KUNIT_EXPECT_TRUE(test, list_is_first(&a, &list)); + KUNIT_EXPECT_FALSE(test, list_is_first(&b, &list)); +} + +static void list_test_list_is_last(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + KUNIT_EXPECT_FALSE(test, list_is_last(&a, &list)); + KUNIT_EXPECT_TRUE(test, list_is_last(&b, &list)); +} + +static void list_test_list_empty(struct kunit *test) +{ + struct list_head a; + LIST_HEAD(list1); + LIST_HEAD(list2); + + list_add_tail(&a, &list1); + + KUNIT_EXPECT_FALSE(test, list_empty(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty(&list2)); +} + +static void list_test_list_empty_careful(struct kunit *test) +{ + /* This test doesn't check correctness under concurrent access */ + struct list_head a; + LIST_HEAD(list1); + LIST_HEAD(list2); + + list_add_tail(&a, &list1); + + KUNIT_EXPECT_FALSE(test, list_empty_careful(&list1)); + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); +} + +static void list_test_list_rotate_left(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + + /* before: [list] -> a -> b */ + list_rotate_left(&list); + /* after: [list] -> b -> a */ + + KUNIT_EXPECT_PTR_EQ(test, list.next, &b); + KUNIT_EXPECT_PTR_EQ(test, b.prev, &list); + KUNIT_EXPECT_PTR_EQ(test, b.next, &a); +} + +static void list_test_list_rotate_to_front(struct kunit *test) +{ + struct list_head a, b, c, d; + struct list_head *list_values[] = { &c, &d, &a, &b }; + struct list_head *ptr; + LIST_HEAD(list); + int i = 0; + + list_add_tail(&a, &list); + list_add_tail(&b, &list); + list_add_tail(&c, &list); + list_add_tail(&d, &list); + + /* before: [list] -> a -> b -> c -> d */ + list_rotate_to_front(&c, &list); + /* after: [list] -> c -> d -> a -> b */ + + list_for_each(ptr, &list) { + KUNIT_EXPECT_PTR_EQ(test, ptr, list_values[i]); + i++; + } + KUNIT_EXPECT_EQ(test, i, 4); +} + +static void list_test_list_is_singular(struct kunit *test) +{ + struct list_head a, b; + LIST_HEAD(list); + + /* [list] empty */ + KUNIT_EXPECT_FALSE(test, list_is_singular(&list)); + + list_add_tail(&a, &list); + + /* [list] -> a */ + KUNIT_EXPECT_TRUE(test, list_is_singular(&list)); + + list_add_tail(&b, &list); + + /* [list] -> a -> b */ + KUNIT_EXPECT_FALSE(test, list_is_singular(&list)); +} + +static void list_test_list_cut_position(struct kunit *test) +{ + struct list_head entries[3], *cur; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&entries[0], &list1); + list_add_tail(&entries[1], &list1); + list_add_tail(&entries[2], &list1); + + /* before: [list1] -> entries[0] -> entries[1] -> entries[2] */ + list_cut_position(&list2, &list1, &entries[1]); + /* after: [list2] -> entries[0] -> entries[1], [list1] -> entries[2] */ + + list_for_each(cur, &list2) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } + + KUNIT_EXPECT_EQ(test, i, 2); + + list_for_each(cur, &list1) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } +} + +static void list_test_list_cut_before(struct kunit *test) +{ + struct list_head entries[3], *cur; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&entries[0], &list1); + list_add_tail(&entries[1], &list1); + list_add_tail(&entries[2], &list1); + + /* before: [list1] -> entries[0] -> entries[1] -> entries[2] */ + list_cut_before(&list2, &list1, &entries[1]); + /* after: [list2] -> entries[0], [list1] -> entries[1] -> entries[2] */ + + list_for_each(cur, &list2) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } + + KUNIT_EXPECT_EQ(test, i, 1); + + list_for_each(cur, &list1) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } +} + +static void list_test_list_splice(struct kunit *test) +{ + struct list_head entries[5], *cur; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&entries[0], &list1); + list_add_tail(&entries[1], &list1); + list_add_tail(&entries[2], &list2); + list_add_tail(&entries[3], &list2); + list_add_tail(&entries[4], &list1); + + /* before: [list1]->e[0]->e[1]->e[4], [list2]->e[2]->e[3] */ + list_splice(&list2, &entries[1]); + /* after: [list1]->e[0]->e[1]->e[2]->e[3]->e[4], [list2] uninit */ + + list_for_each(cur, &list1) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } + + KUNIT_EXPECT_EQ(test, i, 5); +} + +static void list_test_list_splice_tail(struct kunit *test) +{ + struct list_head entries[5], *cur; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&entries[0], &list1); + list_add_tail(&entries[1], &list1); + list_add_tail(&entries[2], &list2); + list_add_tail(&entries[3], &list2); + list_add_tail(&entries[4], &list1); + + /* before: [list1]->e[0]->e[1]->e[4], [list2]->e[2]->e[3] */ + list_splice_tail(&list2, &entries[4]); + /* after: [list1]->e[0]->e[1]->e[2]->e[3]->e[4], [list2] uninit */ + + list_for_each(cur, &list1) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } + + KUNIT_EXPECT_EQ(test, i, 5); +} + +static void list_test_list_splice_init(struct kunit *test) +{ + struct list_head entries[5], *cur; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&entries[0], &list1); + list_add_tail(&entries[1], &list1); + list_add_tail(&entries[2], &list2); + list_add_tail(&entries[3], &list2); + list_add_tail(&entries[4], &list1); + + /* before: [list1]->e[0]->e[1]->e[4], [list2]->e[2]->e[3] */ + list_splice_init(&list2, &entries[1]); + /* after: [list1]->e[0]->e[1]->e[2]->e[3]->e[4], [list2] empty */ + + list_for_each(cur, &list1) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } + + KUNIT_EXPECT_EQ(test, i, 5); + + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); +} + +static void list_test_list_splice_tail_init(struct kunit *test) +{ + struct list_head entries[5], *cur; + LIST_HEAD(list1); + LIST_HEAD(list2); + int i = 0; + + list_add_tail(&entries[0], &list1); + list_add_tail(&entries[1], &list1); + list_add_tail(&entries[2], &list2); + list_add_tail(&entries[3], &list2); + list_add_tail(&entries[4], &list1); + + /* before: [list1]->e[0]->e[1]->e[4], [list2]->e[2]->e[3] */ + list_splice_tail_init(&list2, &entries[4]); + /* after: [list1]->e[0]->e[1]->e[2]->e[3]->e[4], [list2] empty */ + + list_for_each(cur, &list1) { + KUNIT_EXPECT_PTR_EQ(test, cur, &entries[i]); + i++; + } + + KUNIT_EXPECT_EQ(test, i, 5); + + KUNIT_EXPECT_TRUE(test, list_empty_careful(&list2)); +} + +static void list_test_list_entry(struct kunit *test) +{ + struct list_test_struct test_struct; + + KUNIT_EXPECT_PTR_EQ(test, &test_struct, list_entry(&(test_struct.list), + struct list_test_struct, list)); +} + +static void list_test_list_first_entry(struct kunit *test) +{ + struct list_test_struct test_struct1, test_struct2; + LIST_HEAD(list); + + list_add_tail(&test_struct1.list, &list); + list_add_tail(&test_struct2.list, &list); + + + KUNIT_EXPECT_PTR_EQ(test, &test_struct1, list_first_entry(&list, + struct list_test_struct, list)); +} + +static void list_test_list_last_entry(struct kunit *test) +{ + struct list_test_struct test_struct1, test_struct2; + LIST_HEAD(list); + + list_add_tail(&test_struct1.list, &list); + list_add_tail(&test_struct2.list, &list); + + + KUNIT_EXPECT_PTR_EQ(test, &test_struct2, list_last_entry(&list, + struct list_test_struct, list)); +} + +static void list_test_list_first_entry_or_null(struct kunit *test) +{ + struct list_test_struct test_struct1, test_struct2; + LIST_HEAD(list); + + KUNIT_EXPECT_FALSE(test, list_first_entry_or_null(&list, + struct list_test_struct, list)); + + list_add_tail(&test_struct1.list, &list); + list_add_tail(&test_struct2.list, &list); + + KUNIT_EXPECT_PTR_EQ(test, &test_struct1, + list_first_entry_or_null(&list, + struct list_test_struct, list)); +} + +static void list_test_list_next_entry(struct kunit *test) +{ + struct list_test_struct test_struct1, test_struct2; + LIST_HEAD(list); + + list_add_tail(&test_struct1.list, &list); + list_add_tail(&test_struct2.list, &list); + + + KUNIT_EXPECT_PTR_EQ(test, &test_struct2, list_next_entry(&test_struct1, + list)); +} + +static void list_test_list_prev_entry(struct kunit *test) +{ + struct list_test_struct test_struct1, test_struct2; + LIST_HEAD(list); + + list_add_tail(&test_struct1.list, &list); + list_add_tail(&test_struct2.list, &list); + + + KUNIT_EXPECT_PTR_EQ(test, &test_struct1, list_prev_entry(&test_struct2, + list)); +} + +static void list_test_list_for_each(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_test_list_for_each_prev(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); +} + +static void list_test_list_for_each_safe(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); + KUNIT_EXPECT_TRUE(test, list_empty(&list)); +} + +static void list_test_list_for_each_prev_safe(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--; + } + + KUNIT_EXPECT_EQ(test, i, -1); + KUNIT_EXPECT_TRUE(test, list_empty(&list)); +} + +static void list_test_list_for_each_entry(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++; + } + + KUNIT_EXPECT_EQ(test, i, 5); +} + +static void list_test_list_for_each_entry_reverse(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--; + } + + KUNIT_EXPECT_EQ(test, i, -1); +} + +static struct kunit_case list_test_cases[] = { + KUNIT_CASE(list_test_list_init), + KUNIT_CASE(list_test_list_add), + KUNIT_CASE(list_test_list_add_tail), + KUNIT_CASE(list_test_list_del), + KUNIT_CASE(list_test_list_replace), + KUNIT_CASE(list_test_list_replace_init), + KUNIT_CASE(list_test_list_swap), + KUNIT_CASE(list_test_list_del_init), + KUNIT_CASE(list_test_list_move), + KUNIT_CASE(list_test_list_move_tail), + KUNIT_CASE(list_test_list_bulk_move_tail), + KUNIT_CASE(list_test_list_is_first), + KUNIT_CASE(list_test_list_is_last), + KUNIT_CASE(list_test_list_empty), + KUNIT_CASE(list_test_list_empty_careful), + KUNIT_CASE(list_test_list_rotate_left), + KUNIT_CASE(list_test_list_rotate_to_front), + KUNIT_CASE(list_test_list_is_singular), + KUNIT_CASE(list_test_list_cut_position), + KUNIT_CASE(list_test_list_cut_before), + KUNIT_CASE(list_test_list_splice), + KUNIT_CASE(list_test_list_splice_tail), + KUNIT_CASE(list_test_list_splice_init), + KUNIT_CASE(list_test_list_splice_tail_init), + KUNIT_CASE(list_test_list_entry), + KUNIT_CASE(list_test_list_first_entry), + KUNIT_CASE(list_test_list_last_entry), + KUNIT_CASE(list_test_list_first_entry_or_null), + KUNIT_CASE(list_test_list_next_entry), + KUNIT_CASE(list_test_list_prev_entry), + KUNIT_CASE(list_test_list_for_each), + KUNIT_CASE(list_test_list_for_each_prev), + KUNIT_CASE(list_test_list_for_each_safe), + KUNIT_CASE(list_test_list_for_each_prev_safe), + KUNIT_CASE(list_test_list_for_each_entry), + KUNIT_CASE(list_test_list_for_each_entry_reverse), + {}, +}; + +static struct kunit_suite list_test_module = { + .name = "list-kunit-test", + .test_cases = list_test_cases, +}; + +kunit_test_suite(list_test_module);
Hi David,
On 10/24/19 4:46 PM, David Gow wrote:
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h
Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled.
Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry).
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Brendan Higgins brendanhiggins@google.com Tested-by: Brendan Higgins brendanhiggins@google.com
This revision addresses Brendan's comments in https://lore.kernel.org/linux-kselftest/20191023220248.GA55483@google.com/
Specifically:
- Brendan's Reviewed-by/Tested-by being included in the description.
- A couple of trailing tabs in Kconfig.debug & list-test.c
- Reformatting of previously >80 character lines.
Earlier versions of this patchset can be found:
v5: https://lore.kernel.org/linux-kselftest/20191022221322.122788-1-davidgow@goo... v4: https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-davidgow@goog... v3: https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-davidgow@goog... v2: https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-davidgow@goog... v1: https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-davidgow@goog...
CHECK: Unnecessary parentheses around test_struct.list #699: FILE: lib/list-test.c:510: + KUNIT_EXPECT_PTR_EQ(test, &test_struct, list_entry(&(test_struct.list),
CHECK: Alignment should match open parenthesis #700: FILE: lib/list-test.c:511: + KUNIT_EXPECT_PTR_EQ(test, &test_struct, list_entry(&(test_struct.list), + struct list_test_struct, list));
CHECK: Please don't use multiple blank lines #711: FILE: lib/list-test.c:522: + +
CHECK: Alignment should match open parenthesis #713: FILE: lib/list-test.c:524: + KUNIT_EXPECT_PTR_EQ(test, &test_struct1, list_first_entry(&list, + struct list_test_struct, list));
CHECK: Please don't use multiple blank lines #724: FILE: lib/list-test.c:535: + +
CHECK: Alignment should match open parenthesis #726: FILE: lib/list-test.c:537: + KUNIT_EXPECT_PTR_EQ(test, &test_struct2, list_last_entry(&list, + struct list_test_struct, list));
CHECK: Alignment should match open parenthesis #735: FILE: lib/list-test.c:546: + KUNIT_EXPECT_FALSE(test, list_first_entry_or_null(&list, + struct list_test_struct, list));
CHECK: Alignment should match open parenthesis #741: FILE: lib/list-test.c:552: + KUNIT_EXPECT_PTR_EQ(test, &test_struct1, + list_first_entry_or_null(&list,
CHECK: Alignment should match open parenthesis #742: FILE: lib/list-test.c:553: + list_first_entry_or_null(&list, + struct list_test_struct, list));
CHECK: Please don't use multiple blank lines #753: FILE: lib/list-test.c:564: + +
CHECK: Alignment should match open parenthesis #755: FILE: lib/list-test.c:566: + KUNIT_EXPECT_PTR_EQ(test, &test_struct2, list_next_entry(&test_struct1, + list));
CHECK: Please don't use multiple blank lines #766: FILE: lib/list-test.c:577: + +
CHECK: Alignment should match open parenthesis #768: FILE: lib/list-test.c:579: + KUNIT_EXPECT_PTR_EQ(test, &test_struct1, list_prev_entry(&test_struct2, + list));
ERROR: that open brace { should be on the previous line #789: FILE: lib/list-test.c:600: +static void list_test_list_for_each_prev(struct kunit *test) +{
ERROR: that open brace { should be on the previous line #807: FILE: lib/list-test.c:618: +static void list_test_list_for_each_safe(struct kunit *test) +{
CHECK: Please don't use multiple blank lines #813: FILE: lib/list-test.c:624: + +
ERROR: that open brace { should be on the previous line #828: FILE: lib/list-test.c:639: +static void list_test_list_for_each_prev_safe(struct kunit *test) +{
ERROR: that open brace { should be on the previous line #848: FILE: lib/list-test.c:659: +static void list_test_list_for_each_entry(struct kunit *test) +{
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
thanks, -- Shuah
On Tue, Oct 29, 2019 at 6:00 AM shuah shuah@kernel.org wrote:
On 10/24/19 4:46 PM, David Gow wrote:
Add a KUnit test for the kernel doubly linked list implementation in include/linux/list.h
Each test case (list_test_x) is focused on testing the behaviour of the list function/macro 'x'. None of the tests pass invalid lists to these macros, and so should behave identically with DEBUG_LIST enabled and disabled.
Note that, at present, it only tests the list_ types (not the singly-linked hlist_), and does not yet test all of the list_for_each_entry* macros (and some related things like list_prepare_entry).
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Brendan Higgins brendanhiggins@google.com Tested-by: Brendan Higgins brendanhiggins@google.com
This revision addresses Brendan's comments in https://lore.kernel.org/linux-kselftest/20191023220248.GA55483@google.com/
Specifically:
- Brendan's Reviewed-by/Tested-by being included in the description.
- A couple of trailing tabs in Kconfig.debug & list-test.c
- Reformatting of previously >80 character lines.
Earlier versions of this patchset can be found:
v5: https://lore.kernel.org/linux-kselftest/20191022221322.122788-1-davidgow@goo... v4: https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-davidgow@goog... v3: https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-davidgow@goog... v2: https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-davidgow@goog... v1: https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-davidgow@goog...
CHECK: Unnecessary parentheses around test_struct.list #699: FILE: lib/list-test.c:510:
KUNIT_EXPECT_PTR_EQ(test, &test_struct, list_entry(&(test_struct.list),CHECK: Alignment should match open parenthesis #700: FILE: lib/list-test.c:511:
KUNIT_EXPECT_PTR_EQ(test, &test_struct, list_entry(&(test_struct.list),struct list_test_struct, list));CHECK: Please don't use multiple blank lines #711: FILE: lib/list-test.c:522:
CHECK: Alignment should match open parenthesis #713: FILE: lib/list-test.c:524:
KUNIT_EXPECT_PTR_EQ(test, &test_struct1, list_first_entry(&list,struct list_test_struct, list));CHECK: Please don't use multiple blank lines #724: FILE: lib/list-test.c:535:
CHECK: Alignment should match open parenthesis #726: FILE: lib/list-test.c:537:
KUNIT_EXPECT_PTR_EQ(test, &test_struct2, list_last_entry(&list,struct list_test_struct, list));CHECK: Alignment should match open parenthesis #735: FILE: lib/list-test.c:546:
KUNIT_EXPECT_FALSE(test, list_first_entry_or_null(&list,struct list_test_struct, list));CHECK: Alignment should match open parenthesis #741: FILE: lib/list-test.c:552:
KUNIT_EXPECT_PTR_EQ(test, &test_struct1,list_first_entry_or_null(&list,CHECK: Alignment should match open parenthesis #742: FILE: lib/list-test.c:553:
list_first_entry_or_null(&list,struct list_test_struct, list));CHECK: Please don't use multiple blank lines #753: FILE: lib/list-test.c:564:
CHECK: Alignment should match open parenthesis #755: FILE: lib/list-test.c:566:
KUNIT_EXPECT_PTR_EQ(test, &test_struct2, list_next_entry(&test_struct1,list));CHECK: Please don't use multiple blank lines #766: FILE: lib/list-test.c:577:
CHECK: Alignment should match open parenthesis #768: FILE: lib/list-test.c:579:
KUNIT_EXPECT_PTR_EQ(test, &test_struct1, list_prev_entry(&test_struct2,list));ERROR: that open brace { should be on the previous line #789: FILE: lib/list-test.c:600: +static void list_test_list_for_each_prev(struct kunit *test) +{
ERROR: that open brace { should be on the previous line #807: FILE: lib/list-test.c:618: +static void list_test_list_for_each_safe(struct kunit *test) +{
CHECK: Please don't use multiple blank lines #813: FILE: lib/list-test.c:624:
ERROR: that open brace { should be on the previous line #828: FILE: lib/list-test.c:639: +static void list_test_list_for_each_prev_safe(struct kunit *test) +{
ERROR: that open brace { should be on the previous line #848: FILE: lib/list-test.c:659: +static void list_test_list_for_each_entry(struct kunit *test) +{
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
Thanks, Shuah.
Yes, the problem here is that checkpatch.pl believes that anything with "for_each" in its name must be a loop, so expects that the open brace is placed on the same line as for a for loop.
Longer term, I think it'd be nicer, naming-wise, to fix or work around this issue in checkpatch.pl itself, as that'd allow the tests to continue to follow a naming pattern of "list_test_[x]", where [x] is the name of the function/macro being tested. Of course, short of trying to fit a whole C parser in checkpatch.pl, that's going to involve some compromises as well.
In the meantime, I'm sending out v7 which replaces "for_each" with "for__each" (adding the extra underscore), so that checkpatch is happy.
Cheers, -- David
On Wed, Oct 30, 2019 at 01:02:11AM -0700, David Gow wrote:
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
Thanks, Shuah.
Yes, the problem here is that checkpatch.pl believes that anything with "for_each" in its name must be a loop, so expects that the open brace is placed on the same line as for a for loop.
Longer term, I think it'd be nicer, naming-wise, to fix or work around this issue in checkpatch.pl itself, as that'd allow the tests to continue to follow a naming pattern of "list_test_[x]", where [x] is the name of the function/macro being tested. Of course, short of trying to fit a whole C parser in checkpatch.pl, that's going to involve some compromises as well.
Just make it a black list of the 5 most common for_each macros.
In the meantime, I'm sending out v7 which replaces "for_each" with "for__each" (adding the extra underscore), so that checkpatch is happy.
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
regards, dan carpenter
On 10/30/19 4:42 AM, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 01:02:11AM -0700, David Gow wrote:
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
Thanks, Shuah.
Yes, the problem here is that checkpatch.pl believes that anything with "for_each" in its name must be a loop, so expects that the open brace is placed on the same line as for a for loop.
Longer term, I think it'd be nicer, naming-wise, to fix or work around this issue in checkpatch.pl itself, as that'd allow the tests to continue to follow a naming pattern of "list_test_[x]", where [x] is the name of the function/macro being tested. Of course, short of trying to fit a whole C parser in checkpatch.pl, that's going to involve some compromises as well.
Just make it a black list of the 5 most common for_each macros.
How does black listing work in the context of checkpatch.pl?
In the meantime, I'm sending out v7 which replaces "for_each" with "for__each" (adding the extra underscore), so that checkpatch is happy.
This change is required just to quiet checkpatch and I am not happy about asking for this change. At the same time, I am concerned about git hooks failing on this patch.
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
It gets confusing when to ignore and when not to. It takes work to figure out and it is subjective.
It would be great if we can consistently rely on a tool that is used as a criteria for patches to accept patches.
thanks, -- Shuah
On Wed, Oct 30, 2019 at 9:27 AM shuah shuah@kernel.org wrote:
On 10/30/19 4:42 AM, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 01:02:11AM -0700, David Gow wrote:
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
Thanks, Shuah.
Yes, the problem here is that checkpatch.pl believes that anything with "for_each" in its name must be a loop, so expects that the open brace is placed on the same line as for a for loop.
Longer term, I think it'd be nicer, naming-wise, to fix or work around this issue in checkpatch.pl itself, as that'd allow the tests to continue to follow a naming pattern of "list_test_[x]", where [x] is the name of the function/macro being tested. Of course, short of trying to fit a whole C parser in checkpatch.pl, that's going to involve some compromises as well.
Just make it a black list of the 5 most common for_each macros.
How does black listing work in the context of checkpatch.pl?
In the meantime, I'm sending out v7 which replaces "for_each" with "for__each" (adding the extra underscore), so that checkpatch is happy.
This change is required just to quiet checkpatch and I am not happy about asking for this change. At the same time, I am concerned about git hooks failing on this patch.
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
It gets confusing when to ignore and when not to. It takes work to figure out and it is subjective.
It would be great if we can consistently rely on a tool that is used as a criteria for patches to accept patches.
Agreed. I can see the point of not wanting to write an exception into checkpatch for every exception of it's general rules; however, it would be nice if there was a way to maybe have a special comment or something that could turn off a checkpatch error. That way, a checkpatch error/warning always means some action should be taken, and if a rule is being ignored, there is always documentation as to why.
Otherwise, I don't feel strongly about this.
Cheers
On Wed, 2019-10-30 at 09:35 -0700, Brendan Higgins wrote:
Agreed. I can see the point of not wanting to write an exception into checkpatch for every exception of it's general rules; however, it would be nice if there was a way to maybe have a special comment or something that could turn off a checkpatch error. That way, a checkpatch error/warning always means some action should be taken, and if a rule is being ignored, there is always documentation as to why.
That couldn't work when a comment which may exist in a file is out of scope of the patch context.
On Wed, Oct 30, 2019 at 10:18:44AM -0700, Joe Perches wrote:
On Wed, 2019-10-30 at 09:35 -0700, Brendan Higgins wrote:
Agreed. I can see the point of not wanting to write an exception into checkpatch for every exception of it's general rules; however, it would be nice if there was a way to maybe have a special comment or something that could turn off a checkpatch error. That way, a checkpatch error/warning always means some action should be taken, and if a rule is being ignored, there is always documentation as to why.
That couldn't work when a comment which may exist in a file is out of scope of the patch context.
Sorry, I don't understand exactly what you mean. Can you elaborate?
If it wasn't obvious, I am not proposing that David should make the changed I described now for this patch. I know what I proposed would not be an easy thing to implement, especially given the opinions that it is likely to solicit.
Nevertheless, in the long term, I have seen other projects allow a comment that would cause style checkers or static analysis tools to ignore the designated line. Maybe we could implement this as a line comment that suppresses a checkpatch warning of a certain kind on the line. So here, we might have something like:
static void list_test_list_for_each_prev(struct kunit *test) /* checkpatch: disable=for-each-format */
We would also probably want to require an explanation either in the checkpatch comment or the line above, but then you have to worry about that comment not being included in a patch that only updates the offending line.
Anyway, it's just an idea. I know that we don't currently assume that all checkpatch errors/warnings require some action, but it might be cool if they did.
Cheers
On Thu, 2019-10-31 at 01:51 -0700, Brendan Higgins wrote:
On Wed, Oct 30, 2019 at 10:18:44AM -0700, Joe Perches wrote:
On Wed, 2019-10-30 at 09:35 -0700, Brendan Higgins wrote:
Agreed. I can see the point of not wanting to write an exception into checkpatch for every exception of it's general rules; however, it would be nice if there was a way to maybe have a special comment or something that could turn off a checkpatch error. That way, a checkpatch error/warning always means some action should be taken, and if a rule is being ignored, there is always documentation as to why.
That couldn't work when a comment which may exist in a file is out of scope of the patch context.
Sorry, I don't understand exactly what you mean. Can you elaborate?
checkpatch works on patch contexts. If the comment is not within the patch context, checkpatch cannot ignore various test.
static void list_test_list_for_each_prev(struct kunit *test) /* checkpatch: disable=for-each-format */
Long line, now what?
On Wed, Oct 30, 2019 at 10:27:12AM -0600, shuah wrote:
On 10/30/19 4:42 AM, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 01:02:11AM -0700, David Gow wrote:
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
Thanks, Shuah.
Yes, the problem here is that checkpatch.pl believes that anything with "for_each" in its name must be a loop, so expects that the open brace is placed on the same line as for a for loop.
Longer term, I think it'd be nicer, naming-wise, to fix or work around this issue in checkpatch.pl itself, as that'd allow the tests to continue to follow a naming pattern of "list_test_[x]", where [x] is the name of the function/macro being tested. Of course, short of trying to fit a whole C parser in checkpatch.pl, that's going to involve some compromises as well.
Just make it a black list of the 5 most common for_each macros.
How does black listing work in the context of checkpatch.pl?
Hm... I imagined the checkpatch code a little different in my head but this would also work to make it stricter. I doubt it miss very many real life style problems.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a85d719df1f4..4f10e8c0d285 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -3607,7 +3607,7 @@ sub process {
# if/while/etc brace do not go on next line, unless defining a do while loop, # or if that brace on the next line is for something else - if ($line =~ /(.*)\b((?:if|while|for|switch|(?:[a-z_]+|)for_each[a-z_]+)\s*(|do\b|else\b)/ && $line !~ /^.\s*#/) { + if ($line =~ /(.*)\b((?:if|while|for|switch|(?:list|hlist)_for_each[a-z_]+)\s*(|do\b|else\b)/ && $line !~ /^.\s*#/) { my $pre_ctx = "$1$2";
my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
On Wed, 2019-10-30 at 21:46 +0300, Dan Carpenter wrote:
Hm... I imagined the checkpatch code a little different in my head but this would also work to make it stricter. I doubt it miss very many real life style problems.
Well, doubts vs reality...
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
@@ -3607,7 +3607,7 @@ sub process { # if/while/etc brace do not go on next line, unless defining a do while loop, # or if that brace on the next line is for something else
if ($line =~ /(.*)\b((?:if|while|for|switch|(?:[a-z_]+|)for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) {
if ($line =~ /(.*)\b((?:if|while|for|switch|(?:list|hlist)_for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) { my $pre_ctx = "$1$2";my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
So - nak
$ git grep -P '(?:[a-z0-9_]+_)?for_each(?:_[a-z0-9_]+)?' -- '*.[ch]' | \ grep -P -oh '(?:[a-z0-9_]+_)?for_each(?:_[a-z0-9_]+)?' | \ sort | uniq -c | sort -rn | grep -v -P '(?:h?list_| for_each)' 225 netdev_for_each_mc_addr 157 device_for_each_child 132 idr_for_each_entry 101 nla_for_each_nested 96 drm_for_each_connector_iter 91 kvm_for_each_vcpu 88 rdev_for_each 82 scsi_for_each_sg 75 ata_for_each_dev 73 sk_for_each 66 bus_for_each_dev 64 xa_for_each 62 hash_for_each_possible 59 shost_for_each_device 54 idr_for_each 54 efx_for_each_channel 48 hash_for_each_safe 48 ata_for_each_link 47 ef4_for_each_channel 46 snd_pcm_group_for_each_entry 46 bond_for_each_slave 44 netdev_for_each_uc_addr 44 drm_for_each_crtc 39 rbtree_postorder_for_each_entry_safe 38 xas_for_each 37 bio_for_each_segment_all 36 of_property_for_each_string 36 bio_for_each_segment 32 radix_tree_for_each_slot 31 class_for_each_device 30 queue_for_each_hw_ctx 29 rq_for_each_segment 28 of_property_for_each_u32 28 drm_connector_for_each_possible_encoder 27 drm_atomic_for_each_plane_damage 27 bond_for_each_slave_rcu 26 snd_array_for_each 26 rdma_for_each_port 26 drm_for_each_encoder 26 device_for_each_child_node 25 ubi_rb_for_each_entry 24 driver_for_each_device 23 nla_for_each_attr 23 hash_for_each 23 drm_for_each_plane 21 xas_for_each_marked 21 snd_soc_dapm_widget_for_each_sink_path 21 pci_bus_for_each_resource 21 drm_mm_for_each_node_safe 20 __shost_for_each_device 20 mdesc_for_each_node_by_name 20 mdesc_for_each_arc 20 iscsi_host_for_each_session 19 media_device_for_each_entity 19 drm_client_for_each_modeset 19 bus_for_each_drv 18 pci_for_each_dma_alias 18 nexthop_for_each_fib6_nh 18 netdev_for_each_lower_dev 18 label_for_each 18 in_dev_for_each_ifa_rcu 18 efx_for_each_channel_tx_queue 17 starget_for_each_device 17 snd_soc_dapm_widget_for_each_source_path 17 hash_for_each_possible_rcu 17 eeh_for_each_pe 17 drm_for_each_plane_mask 17 css_for_each_descendant_pre 16 wl12xx_for_each_wlvif 16 rhl_for_each_entry_rcu 16 ice_for_each_traffic_class 16 fn_for_each_confined 16 ebitmap_for_each_positive_bit 16 bpf_object__for_each_program 15 tx_queue_for_each 15 fn_for_each 15 flow_action_for_each 15 ef4_for_each_channel_tx_queue 15 eeh_pe_for_each_dev 15 drm_for_each_encoder_mask 15 cpufreq_for_each_valid_entry 14 rdev_for_each_rcu 14 kvm_for_each_memslot 14 ice_for_each_vsi 14 hists__for_each_format 13 xa_for_each_marked 13 pwrdm_for_each 13 klp_for_each_object 13 in_dev_for_each_ifa_rtnl 13 ice_for_each_q_vector 13 efx_for_each_channel_rx_queue 13 drm_for_each_detailed_block 13 cgroup_taskset_for_each 12 v4l2_device_for_each_subdev 12 __rq_for_each_bio 12 rcu_for_each_leaf_node 12 drm_atomic_crtc_for_each_plane 11 wl12xx_for_each_wlvif_sta 11 while_for_each_ftrace_op 11 sctp_skb_for_each 11 queue_for_each 11 qed_for_each_vf 11 klp_for_each_func 11 iommu_group_for_each_dev 11 ide_port_for_each_present_dev 11 i2c_for_each_dev 11 hash_for_each_rcu 11 ef4_for_each_channel_rx_queue 11 do_for_each_ftrace_rec 11 cpufreq_for_each_valid_entry_idx 11 cpufreq_for_each_entry 10 while_for_each_ftrace_rec 10 sk_for_each_bound 10 rpc_clnt_iterate_for_each_xprt 10 of_for_each_phandle 10 input_handler_for_each_handle 10 drm_mm_for_each_node 10 do_for_each_ftrace_op 10 clkdm_for_each 10 bpf_object__for_each_map 9 vlan_group_for_each_dev 9 vlan_for_each 9 toptree_for_each 9 perf_event_for_each_child 9 ixgbe_for_each_ring 9 io_wq_for_each_worker 9 ide_port_for_each_dev 9 i3c_bus_for_each_i3cdev 9 dp_for_each_set_bit 9 ax25_for_each 8 vfio_pci_for_each_slot_or_bus 8 usbhs_for_each_pipe_with_dcp 8 usb_for_each_dev 8 snd_soc_dapm_widget_for_each_path 8 sk_nulls_for_each 8 sk_for_each_rcu 8 rt2x00queue_for_each_entry 8 rht_for_each_from 8 rcu_for_each_node_breadth_first 8 omap_hwmod_for_each 8 netdev_for_each_tx_queue 8 ide_host_for_each_port 8 hashmap__for_each_key_entry 8 hashmap__for_each_entry 8 fn_for_each_in_ns 8 fdt_for_each_subnode 8 drm_mm_for_each_hole 8 drm_atomic_crtc_state_for_each_plane 8 device_for_each_child_reverse 8 cpuset_for_each_descendant_pre 8 __bio_for_each_segment 7 vsock_for_each_connected_socket 7 tcf_exts_for_each_action 7 snd_soc_dapm_for_each_direction 7 sk_nulls_for_each_rcu 7 sctp_for_each_hentry 7 rdma_for_each_block 7 radix_tree_for_each_tagged 7 pnp_for_each_dev 7 pack_for_each_init 7 netdev_for_each_upper_dev_rcu 7 mtd_for_each_device 7 mlx5e_for_each_hash_node 7 key_for_each 7 kcore_copy__for_each_phdr 7 ixgbevf_for_each_ring 7 intel_atomic_crtc_state_for_each_plane_state 7 idr_for_each_entry_continue 7 i3c_bus_for_each_i2cdev 7 hsr_for_each_port 7 fdt_for_each_property_offset 7 drm_atomic_crtc_state_for_each_plane_state 7 dma_fence_chain_for_each 7 data__for_each_file 7 blkg_for_each_descendant_pre 7 ath_for_each_chanctx 7 ata_qc_for_each_raw 6 xa_for_each_start 6 while_for_each_event_file 6 v4l2_m2m_for_each_src_buf_safe 6 uwb_dev_for_each 6 usb_hub_for_each_child 6 __starget_for_each_device 6 sbitmap_for_each_set 6 perf_config_items__for_each_entry 6 page_chain_for_each 6 omap2_clk_for_each 6 nlmsg_for_each_attr 6 map__for_each_symbol 6 machine__for_each_thread 6 label_for_each_in_ns 6 label_for_each_confined 6 ipmr_for_each_table 6 ice_for_each_txq 6 ice_for_each_rxq 6 func_for_each_insn_all 6 dpm_for_each_dev 6 do_for_each_event_file 6 dfl_fpga_dev_for_each_feature 6 css_for_each_descendant_post 6 css_for_each_child 6 cpuset_for_each_child 6 cgroup_for_each_live_descendant_pre 6 blkg_for_each_descendant_post 6 at91_for_each_port 5 wl12xx_for_each_wlvif_ap 5 usb3_for_each_dma 5 unwind_for_each_frame 5 udp_portaddr_for_each_entry_rcu 5 trace_probe_for_each_link_rcu 5 rq_for_each_bvec 5 rht_for_each_rcu 5 rds_for_each_conn_info 5 ppr_for_each_entry 5 omap_hwmod_for_each_by_class 5 nouveau_for_each_non_mst_connector_iter 5 nft_rule_for_each_expr 5 label_for_each_cont 5 klp_for_each_patch 5 ip6mr_for_each_table 5 __iommu_group_for_each_dev 5 intel_gvt_for_each_tracked_mmio 5 inet_bind_bucket_for_each 5 idr_for_each_entry_ul 5 ice_for_each_ring 5 i40e_for_each_ring 5 hashmap__for_each_entry_safe 5 hash_for_each_possible_safe 5 gen_pool_for_each_chunk 5 gadget_for_each_ep 5 fwnode_for_each_child_node 5 fm10k_for_each_ring 5 fec_for_each_buffer_rs_block 5 enclosure_for_each_device 5 drm_client_for_each_connector_iter 5 drbd_for_each_overlap 5 devres_for_each_res 5 db_for_each_64 5 cpufreq_for_each_entry_idx 5 bpf_for_each_spilled_reg 5 bip_for_each_vec 5 bio_for_each_bvec 5 b53_for_each_port 5 ax25_uid_for_each 4 xen_for_each_gfn 4 xan_for_each_marked 4 wpan_phy_for_each 4 v4l2_m2m_for_each_dst_buf_safe 4 usbhs_for_each_dfifo 4 toptree_for_each_child_safe 4 target_for_each_device 4 ssb_for_each_bus_call 4 srcu_for_each_node_breadth_first 4 sk_for_each_safe 4 sk_for_each_from 4 sk_for_each_entry_offset_rcu 4 sctp_for_each_transport 4 sctp_for_each_endpoint 4 __sbitmap_for_each_set 4 resort_rb__for_each_entry 4 register_for_each_vma 4 rdev_for_each_list 4 rbt_ib_umem_for_each_in_range 4 qeth_for_each_output_queue 4 ping_portaddr_for_each_entry 4 perf_event_for_each 4 perf_cpu_map__for_each_cpu 4 perf_config_set__for_each_entry 4 pcpu_for_each_unpop_region 4 nr_node_for_each_safe 4 nr_node_for_each 4 nlmsg_for_each_msg 4 nfp_for_each_insn_walk2 4 __neigh_for_each_release 4 neigh_for_each 4 mlxsw_sp_rif_neigh_for_each 4 mlxsw_sp_nexthop_for_each 4 idr_for_each_entry_continue_ul 4 hists__for_each_sort_list 4 hctx_for_each_ctx 4 fwnode_for_each_available_child_node 4 fs_for_each_prio 4 elf_symtab__for_each_symbol 4 efx_for_each_possible_channel_tx_queue 4 ef4_for_each_possible_channel_tx_queue 4 drm_mm_for_each_node_in_range 4 drm_for_each_legacy_plane 4 drm_for_each_fb 4 do_for_each_event_file_safe 4 cma_for_each_area 4 chp_id_for_each 4 cgroup_taskset_for_each_leader 4 cgroup_for_each_live_descendant_post 4 card_for_each_dev 4 bt_tags_for_each 4 __btree_for_each 4 bt_for_each 4 bpf_object__for_each_safe 4 __ata_qc_for_each 4 ata_qc_for_each 4 apei_exec_for_each_entry 4 apei_estatus_for_each_section 4 amdgpu_umc_for_each_channel 4 acpi_nvs_for_each_region 3 xas_for_each_conflict 3 wl12xx_for_each_wlvif_bss_type 3 vnic_hash_for_each 3 v4l2_m2m_for_each_dst_buf 3 uwb_dev_for_each_f 3 __usbhsh_for_each_udev 3 usbhsg_for_each_uep_with_dcp 3 __usbhsg_for_each_uep 3 __usbhs_for_each_pipe 3 usb3_for_each_ep 3 ubi_for_each_used_peb 3 ubi_for_each_protected_peb 3 ubi_for_each_free_peb 3 txall_queue_for_each 3 toptree_for_each_safe 3 toptree_for_each_child 3 tcm_for_each_slice 3 symbols__for_each_entry 3 simple_for_each_link 3 shdma_for_each_chan 3 sec_for_each_insn_from 3 sec_for_each_insn 3 sctp_for_each_tx_datachunk 3 sb_for_each_fn 3 rocker_tlv_for_each 3 rht_for_each_rcu_from 3 rht_for_each_entry_rcu_from 3 rht_for_each_entry_from 3 reiserfs_for_each_xattr 3 rdev_for_each_safe 3 rcar_thermal_for_each_priv 3 perf_event_groups_for_each 3 pcpu_for_each_md_free_region 3 pcpu_for_each_fit_region 3 nr_neigh_for_each_safe 3 nr_neigh_for_each 3 nanddev_io_for_each_page 3 mtrr_for_each_mem_type 3 mlxsw_sp_prefix_usage_for_each 3 mlxsw_afk_element_usage_for_each 3 mlx5_esw_for_each_vf_vport_num_reverse 3 mlx5_esw_for_each_host_func_vport 3 mlx5e_for_each_arfs_rule 3 media_device_for_each_intf 3 machines__for_each_thread 3 iov_iter_for_each_range 3 inet_lhash2_for_each_icsk_rcu 3 ice_for_each_alloc_txq 3 ice_for_each_alloc_rxq 3 iavf_for_each_ring 3 hns3_for_each_ring 3 graph_for_each_link 3 gmap_for_each_rmap_safe 3 genradix_for_each 3 gen6_for_each_pde 3 fwnode_graph_for_each_endpoint 3 fs_for_each_ft 3 fq_ring_for_each 3 __for_each_sgt_daddr 3 fn_for_each_not_in_set 3 fifo_for_each 3 fec_for_each_prealloc_buffer 3 fec_for_each_extra_buffer 3 fec_for_each_buffer 3 dsos__for_each_with_build_id 3 dso__for_each_symbol 3 drm_for_each_privobj 3 drm_for_each_lessee 3 data__for_each_file_start 3 data__for_each_file_new 3 dapm_kcontrol_for_each_path 3 btree_for_each_safe64 3 btree_for_each_safe32 3 btf_for_each_str_off 3 bpf__for_each_map_named 3 bio_for_each_integrity_vec 3 ata_qc_for_each_with_internal 3 arm_ccn_for_each_valid_region 2 zorro_for_each_dev 2 wl12xx_for_each_wlvif_continue 2 vtb_for_each_detailed_block 2 vnic_hash_for_each_safe 2 vnic_hash_for_each_possible 2 virtio_device_for_each_vq 2 v4l2_m2m_for_each_src_buf 2 usbhsh_for_each_udev_with_dev0 2 usbhsh_for_each_udev 2 udp_portaddr_for_each_entry 2 ubi_for_each_scrub_peb 2 trace_probe_for_each_link 2 test_for_each_set_clump8 2 tb_property_for_each 2 snd_soc_dapm_widget_for_each_path_safe 2 sk_nulls_for_each_from 2 sctp_for_each_rx_skb 2 scsi_for_each_prot_sg 2 rocker_tlv_for_each_nested 2 rht_for_each_entry_safe 2 rht_for_each_entry_rcu 2 rht_for_each_entry 2 rht_for_each 2 rhl_for_each_rcu 2 protocol_for_each_dev 2 protocol_for_each_card 2 pnp_for_each_card 2 perf_config_sections__for_each_entry 2 pcpu_for_each_pop_region 2 pci_mmcfg_for_each_region 2 page_chain_for_each_safe 2 of_property_for_each_string_index 2 nfp_for_each_insn_walk3 2 netdev_for_each_lower_private_rcu 2 netdev_for_each_lower_private 2 mlx5_esw_for_each_vf_vport_num 2 mlx5_esw_for_each_vf_vport 2 mlx5_esw_for_each_vf_rep_reverse 2 mlx5_esw_for_each_vf_rep 2 mlx5_esw_for_each_host_func_vport_reverse 2 mlx5_esw_for_each_host_func_rep 2 mlx5e_for_each_hash_arfs_rule 2 media_device_for_each_pad 2 media_device_for_each_link 2 __map__for_each_symbol_by_name 2 map__for_each_symbol_by_name 2 libbpf_nla_for_each_attr 2 __labelset_for_each 2 label_for_each_in_merge 2 label_for_each_comb 2 klp_for_each_patch_safe 2 klp_for_each_object_static 2 klp_for_each_object_safe 2 klp_for_each_func_static 2 klp_for_each_func_safe 2 key_for_each_safe 2 hashmap__for_each_key_entry_safe 2 hash_for_each_possible_rcu_notrace 2 genradix_for_each_from 2 func_for_each_insn_continue_reverse 2 func_for_each_insn 2 fs_for_each_ns 2 fs_for_each_fg 2 __for_each_thread 2 __for_each_child_of_node 2 elf_section__for_each_rela 2 elf_section__for_each_rel 2 efx_for_each_channel_rev 2 ef4_for_each_channel_rev 2 drm_crtc_for_each_plane 2 devcom_for_each_component 2 cgroup_for_each_live_child 2 cea_for_each_detailed_block 2 carl9170fw_for_each_hdr 2 bpf__for_each_map 2 __bio_for_each_bvec 1 usbhsg_for_each_uep 1 usbhs_for_each_pipe 1 ubi_for_each_scub_peb 1 toptree_for_each_sibling 1 snd_soc_dapm_widget_for_each_sink_path_safe 1 sec_for_each_insn_continue 1 rmi_for_each_dev 1 pwrdm_for_each_nolock 1 pwrdm_for_each_clkdm 1 pneigh_for_each 1 perf_config_set__for_each 1 perf_config_sections__for_each 1 perf_config_items__for_each 1 nilfs_for_each_segbuf_before 1 nand_io_for_each_page 1 mlx5_esw_for_each_vf_vport_reverse 1 mlx5_esw_for_each_host_func_rep_reverse 1 map__for_each_symbol_with_name 1 label_for_each_not_in_set 1 gmap_for_each_rmap 1 fs_for_each_ns_or_ft_reverse 1 fs_for_each_ns_or_ft 1 fs_for_each_ft_safe 1 fs_for_each_fte 1 fs_for_each_dst 1 fn_for_each_in_merge 1 fn_for_each_comb 1 drm_crtc_atomic_state_for_each_plane_state 1 drm_crtc_atomic_state_for_each_plane 1 class_for_each_dev 1 bus_for_each 1 btree_for_each_safel 1 btree_for_each_safe128 1 bpf_map__for_each 1 blk_queue_for_each_rl
On Wed, Oct 30, 2019 at 12:15:30PM -0700, Joe Perches wrote:
On Wed, 2019-10-30 at 21:46 +0300, Dan Carpenter wrote:
Hm... I imagined the checkpatch code a little different in my head but this would also work to make it stricter. I doubt it miss very many real life style problems.
Well, doubts vs reality...
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
@@ -3607,7 +3607,7 @@ sub process { # if/while/etc brace do not go on next line, unless defining a do while loop, # or if that brace on the next line is for something else
if ($line =~ /(.*)\b((?:if|while|for|switch|(?:[a-z_]+|)for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) {
if ($line =~ /(.*)\b((?:if|while|for|switch|(?:list|hlist)_for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) { my $pre_ctx = "$1$2";my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
So - nak
What I mean is that only the people doing list_for_each and hlist_for_each don't know how to do it right. I just tested this over night and my assumptions were correct. Here are all the lines that generate a warning:
+ hlist_for_each_entry_safe(tmp_fil, n, head, fnode) +static void list_test_list_for_each_prev(struct kunit *test) +static void list_test_list_for_each_safe(struct kunit *test) +static void list_test_list_for_each_prev_safe(struct kunit *test) +static void list_test_list_for_each_entry(struct kunit *test) +static void list_test_list_for_each_entry_reverse(struct kunit *test) + hlist_for_each_entry_safe(x6spi, n, + list_for_each_entry(w, &card->widgets, list)
Only the first and last warnings are real style problems and my patch catches both.
regards, dan carpenter
On 30/10/2019 20.15, Joe Perches wrote:
On Wed, 2019-10-30 at 21:46 +0300, Dan Carpenter wrote:
Hm... I imagined the checkpatch code a little different in my head but this would also work to make it stricter. I doubt it miss very many real life style problems.
Well, doubts vs reality...
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
@@ -3607,7 +3607,7 @@ sub process { # if/while/etc brace do not go on next line, unless defining a do while loop, # or if that brace on the next line is for something else
if ($line =~ /(.*)\b((?:if|while|for|switch|(?:[a-z_]+|)for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) {
if ($line =~ /(.*)\b((?:if|while|for|switch|(?:list|hlist)_for_each[a-z_]+)\s*\(|do\b|else\b)/ && $line !~ /^.\s*\#/) { my $pre_ctx = "$1$2";my ($level, @ctx) = ctx_statement_level($linenr, $realcnt, 0);
So - nak
How about changing the check so it only matches the if/while/for/*for_each*/ thing when it's the first thing on a line _and_ has non-trivial whitespace in front. Then a function declaration as
static void test_for_each() {
would not fire, nor would it if it were written in the other common style
static void test_for_each() {
?
Maybe there'd still be a problem at the call-sites
test_for_each(); this_is_not_indented;
but the ending semi-colon should actually make it appear as a loop with an empty body (though that in itself might fire a different warning, dunno if checkpatch has that kind of warnings). But in any case the above should remove _some_ false positives.
Rasmus
On Wed, Oct 30, 2019 at 10:27:12AM -0600, shuah wrote:
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
It gets confusing when to ignore and when not to. It takes work to figure out and it is subjective.
In this case, it's not subjective because checkpatch is clearly not working as intended.
I don't feel like "checkpatch clean" is a useful criteria for applying patches. If someone sends a patch and I can spot a bunch of checkpatch issues with my bare eyeballs then I get slightly annoyed for wasting my time. But as a reviewer, I mostly care about my own judgement. Can I understand what the code is doing? It is subjective, but I'm smarter than a Perl script and I try to be kind to people.
The other things about warnings is that I always encourage people to just ignore old warnings. If you're running Smatch and you see a warning in ancient code that means I saw it five years ago and didn't fix it so it's a false positive. Old warnings are always 100% false positives.
regards, dan carpenter
On Wed, 2019-10-30 at 22:12 +0300, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 10:27:12AM -0600, shuah wrote:
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
It gets confusing when to ignore and when not to. It takes work to figure out and it is subjective.
In this case, it's not subjective because checkpatch is clearly not working as intended.
checkpatch _is_ working as intended. It was never intended to be perfect.
checkpatch _always_ depended on a reviewer deciding whether its output was appropriate.
I don't feel like "checkpatch clean" is a useful criteria for applying patches.
Nor do I.
The other things about warnings is that I always encourage people to just ignore old warnings. If you're running Smatch and you see a warning in ancient code that means I saw it five years ago and didn't fix it so it's a false positive. Old warnings are always 100% false positives.
That'd be not absolute either because it depended on your historical judgment as to whether an old warning was in fact a defect or not.
People make mistakes. Regex based scripts are by design stupid and untrustworthy.
Mistakes will be made. Just fix the actual defects in code as soon as possible.
I tend to agree that it's better to either fix or ignore checkpatch than to arbitrarily change things in cases like this where checkpatch is obviously wrong. Equally, it certainly seems that there isn't an obvious way of modifying checkpatch that will both not cause other problems and not add another arbitrary name check. The main concern about just leaving the checkpatch errors in is that people might be automatically rejecting changes (or worse, the whole kselftest/test pull request) if checkpatch errors are present. I'm not sure how likely that is, but I can understand the desire to be careful, since relatively minor changes have delayed KUnit changes before.
So, there are a few options, I guess: - Hack around the issue in the patch (as this v7 is doing). Ugly, but does at least mean that this change won't trigger any automated rejection-of-checkpatch-errors people might be doing. (Even if, I think we agree, automatically rejecting anything with checkpatch warnings is not really correct.) - Accept that tests (and other functions) with "for_each" in the name like this are rare enough that it's not worth the complexity of supporting it in checkpatch, and taking v6 as-is with the checkpatch errors. - Modify checkpatch to handle this in some other way (e.g., only if the name doesn't include "test"): I don't think there's a perfectly clean way of doing this. - Modify checkpatch to make this ERROR a WARNING instead, since we know this check has some flaws in this test, and potentially future tests. - Re-send v6 with a note about the checkpatch warning in the description, so that it's easier to tell if one or more of these
Is there some combination of the above that sounds good?
-- David
David, this is an easy question to answer. I think Shuah is the maintainer here? You don't have to make everyone happy, you just have to make Shuah happy. Joe and I have very little emotional investment in this code and we don't care what you do and even if we did, it wouldn't matter.
regards, dan carpenter
On 10/30/19 1:23 PM, Joe Perches wrote:
On Wed, 2019-10-30 at 22:12 +0300, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 10:27:12AM -0600, shuah wrote:
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
It gets confusing when to ignore and when not to. It takes work to figure out and it is subjective.
In this case, it's not subjective because checkpatch is clearly not working as intended.
checkpatch _is_ working as intended. It was never intended to be perfect.
checkpatch _always_ depended on a reviewer deciding whether its output was appropriate.
I don't feel like "checkpatch clean" is a useful criteria for applying patches.
Nor do I.
The other things about warnings is that I always encourage people to just ignore old warnings. If you're running Smatch and you see a warning in ancient code that means I saw it five years ago and didn't fix it so it's a false positive. Old warnings are always 100% false positives.
That'd be not absolute either because it depended on your historical judgment as to whether an old warning was in fact a defect or not.
People make mistakes. Regex based scripts are by design stupid and untrustworthy.
Mistakes will be made. Just fix the actual defects in code as soon as possible.
Thanks all for chiming in. I am taking v6 as is and adding an update to commit log capture the spurious errors from checkpath.pl for this specific case.
thanks, -- Shuah
On Wed, 2019-10-30 at 13:42 +0300, Dan Carpenter wrote:
On Wed, Oct 30, 2019 at 01:02:11AM -0700, David Gow wrote:
ERROR: that open brace { should be on the previous line #869: FILE: lib/list-test.c:680: +static void list_test_list_for_each_entry_reverse(struct kunit *test) +{
I am seeing these error and warns. As per our hallway conversation, the "for_each*" in the test naming is tripping up checkpatch.pl
For now you can change the name a bit to not trip checkpatch and maybe explore fixing checkpatch to differentiate between function names with "for_each" in them vs. the actual for_each usages in the code.
[]
It's better to ignore checkpatch and other scripts when they are wrong. (unless the warning message inspires you to make the code more readable for humans).
True.
On Thu, Oct 24, 2019 at 03:46:31PM -0700, David Gow wrote:
diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..f3d0c6e42b97 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,13 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c +LIST KUNIT TEST +M: David Gow davidgow@google.com +L: linux-kselftest@vger.kernel.org +L: kunit-dev@googlegroups.com +S: Maintained +F: lib/list-test.c
Should KUnit be the first name here? Then all KUnit tests appear in the same location in the MAINTAINERS file, or should it be like it is here, so that KUnit tests are close to the same-named area?
LIVE PATCHING M: Josh Poimboeuf jpoimboe@redhat.com M: Jiri Kosina jikos@kernel.org diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..6c1be6181e38 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST If unsure, say N. +config LIST_KUNIT_TEST
Similarly for the Kconfig name. (Also aren't KUNIT and TEST redundant?)
config KUNIT_LIST
?
config LIST_KUNIT
--- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o
+# KUnit tests +obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
And again, list-kunit.o? Other things have -test (or more commonly _test) suffixes. (So maybe list_kunit.o?)
But as I said last time, I'll live with whatever, I'd just like a documented best-practice with a reasonable rationale. :)
On Thu, Oct 31, 2019 at 11:51 AM Kees Cook keescook@chromium.org wrote:
On Thu, Oct 24, 2019 at 03:46:31PM -0700, David Gow wrote:
diff --git a/MAINTAINERS b/MAINTAINERS index 7ef985e01457..f3d0c6e42b97 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9504,6 +9504,13 @@ F: Documentation/misc-devices/lis3lv02d.rst F: drivers/misc/lis3lv02d/ F: drivers/platform/x86/hp_accel.c
+LIST KUNIT TEST +M: David Gow davidgow@google.com +L: linux-kselftest@vger.kernel.org +L: kunit-dev@googlegroups.com +S: Maintained +F: lib/list-test.c
Should KUnit be the first name here? Then all KUnit tests appear in the same location in the MAINTAINERS file, or should it be like it is here, so that KUnit tests are close to the same-named area?
Thus far, we haven't standardised on anything re: MAINTAINERS entries for tests. For the sysctl test, for instance, the file has been added to the general "PROC SYSCTL" section. There's no existing MAINTAINERS entry for list.h at all, though, so that's couldn't be done here.
My suspicion is that it doesn't matter all that much (isn't everyone just grepping MAINTAINERS anyway?), but that long-term, tests are more likely to be being maintained in parallel with the code under test, rather than in one group block of tests. I don't mind changing it if anyone has stronger opinions, though...
LIVE PATCHING M: Josh Poimboeuf jpoimboe@redhat.com M: Jiri Kosina jikos@kernel.org diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index a3017a5dadcd..6c1be6181e38 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1961,6 +1961,24 @@ config SYSCTL_KUNIT_TEST
If unsure, say N.+config LIST_KUNIT_TEST
Similarly for the Kconfig name. (Also aren't KUNIT and TEST redundant?)
config KUNIT_LIST
?
config LIST_KUNIT
This matches what's being done with the existing sysctl test, which uses SYSCTL_KUNIT_TEST as its config name. So, we've kind-of standardised on x_KUNIT_TEST thus far, even if it is a bit redundant.
--- a/lib/Makefile +++ b/lib/Makefile @@ -292,3 +292,6 @@ obj-$(CONFIG_GENERIC_LIB_MULDI3) += muldi3.o obj-$(CONFIG_GENERIC_LIB_CMPDI2) += cmpdi2.o obj-$(CONFIG_GENERIC_LIB_UCMPDI2) += ucmpdi2.o obj-$(CONFIG_OBJAGG) += objagg.o
+# KUnit tests +obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
And again, list-kunit.o? Other things have -test (or more commonly _test) suffixes. (So maybe list_kunit.o?)
But as I said last time, I'll live with whatever, I'd just like a documented best-practice with a reasonable rationale. :)
Similarly, we've been going with a -test suffix thus far.
I definitely agree that these conventions should be documented, though.
Cheers, -- David
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Dan Carpenter -
David Gow -
Joe Perches -
Kees Cook -
Rasmus Villemoes -
shuah