[PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[David Gow]

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@google.com/
v4:
https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-davidgow@google.com/
v3:
https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-davidgow@google.com/
v2:
https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-davidgow@google.com/
v1:
https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-davidgow@google.com/

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);
-- 
2.24.0.rc0.303.g954a862665-goog

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[shuah]

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@google.com/
> v4:
> https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-davidgow@google.com/
> v3:
> https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-davidgow@google.com/
> v2:
> https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-davidgow@google.com/
> v1:
> https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-davidgow@google.com/
> 

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[David Gow]

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@google.com/
> > v4:
> > https://lore.kernel.org/linux-kselftest/20191018215549.65000-1-davidgow@google.com/
> > v3:
> > https://lore.kernel.org/linux-kselftest/20191016215707.95317-1-davidgow@google.com/
> > v2:
> > https://lore.kernel.org/linux-kselftest/20191010185631.26541-1-davidgow@google.com/
> > v1:
> > https://lore.kernel.org/linux-kselftest/20191007213633.92565-1-davidgow@google.com/
> >
>
> 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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Dan Carpenter]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[shuah]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Joe Perches]

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.

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Brendan Higgins]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Joe Perches]

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.

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Dan Carpenter]

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);

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Dan Carpenter]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Joe Perches]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Joe Perches]

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.

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Dan Carpenter]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[David Gow]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Dan Carpenter]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Brendan Higgins]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Joe Perches]

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?

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Dan Carpenter]

On Thu, Oct 31, 2019 at 01:51:29AM -0700, Brendan Higgins wrote:
> static void list_test_list_for_each_prev(struct kunit *test) /* checkpatch: disable=for-each-format */

These comments defeat the purpose of checkpatch which is to make the
code cleaner.

regards,
dan carpenter

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Kees Cook]

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. :)

-- 
Kees Cook

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[David Gow]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[Rasmus Villemoes]

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

Re: [PATCH linux-kselftest/test v6] lib/list-test: add a test for the 'list' doubly linked list

[shuah]

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