| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABVgOS=Vhigp8bHVij9kRyEZDzDzLafz2O6LT471RSLHctRsGg@mail.gmail.com>
Date: Thu, 30 Mar 2023 14:15:22 +0800
From: David Gow <davidgow@...gle.com>
To: Sadiya Kazi <sadiyakazi@...gle.com>
Cc: brendanhiggins@...gle.com, skhan@...uxfoundation.org,
corbet@....net, linux-kselftest@...r.kernel.org,
kunit-dev@...glegroups.com, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] list: test: Test the klist structure
On Wed, 29 Mar 2023 at 19:07, Sadiya Kazi <sadiyakazi@...gle.com> wrote:
>
> Add KUnit tests to the klist linked-list structure.
> These perform testing for different variations of node add
> and node delete in the klist data structure (<linux/klist.h>).
>
> Limitation: Since we use a static global variable, and if
> multiple instances of this test are run concurrently, the test may fail.
>
> Signed-off-by: Sadiya Kazi <sadiyakazi@...gle.com>
> ---
Thanks very much! This will be a great addition to the list tests (and
the device model tests).
A couple of super-minor suggestions re: comment formatting, otherwise
good-to-go.
Reviewed-by: David Gow <davidgow@...gle.com>
Cheers,
-- David
> lib/list-test.c | 298 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 297 insertions(+), 1 deletion(-)
>
> diff --git a/lib/list-test.c b/lib/list-test.c
> index d374cf5d1a57..af651cacb8f4 100644
> --- a/lib/list-test.c
> +++ b/lib/list-test.c
> @@ -8,6 +8,7 @@
> #include <kunit/test.h>
>
> #include <linux/list.h>
> +#include <linux/klist.h>
>
> struct list_test_struct {
> int data;
> @@ -1199,6 +1200,301 @@ static struct kunit_suite hlist_test_module = {
> .test_cases = hlist_test_cases,
> };
>
> -kunit_test_suites(&list_test_module, &hlist_test_module);
> +
> +struct klist_test_struct {
> + int data;
> + struct klist klist;
> + struct klist_node klist_node;
> +};
> +
> +/* counts the number of nodes*/
This comment doesn't really add anything,
> +static int node_count;
> +static struct klist_node *last_node_count;
This isn't a count... can we rename it?
> +
> +static void check_node(struct klist_node *node_ptr)
> +{
> + node_count++;
> + last_node_count = node_ptr;
> +}
> +
> +static void check_delete_node(struct klist_node *node_ptr)
> +{
> + node_count--;
> + last_node_count = node_ptr;
> +}
> +
> +static void klist_test_add_tail(struct kunit *test)
> +{
> + struct klist_node a, b;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_tail(&a, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 1);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &a);
> +
> + klist_add_tail(&b, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 2);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &b);
> +
> + /* should be [list] -> a -> b */
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_add_head(struct kunit *test)
> +{
> + struct klist_node a, b;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_head(&a, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 1);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &a);
> +
> + klist_add_head(&b, &mylist);
> + KUNIT_EXPECT_EQ(test, node_count, 2);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &b);
> +
> + /* should be [list] -> b -> a */
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_add_behind(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_head(&a, &mylist);
> + klist_add_head(&b, &mylist);
> +
> + klist_add_behind(&c, &a);
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + klist_add_behind(&d, &b);
> + KUNIT_EXPECT_EQ(test, node_count, 4);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &d);
> +
> + klist_iter_init(&mylist, &i);
> +
> + /* should be [list] -> b -> d -> a -> c*/
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &c);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_add_before(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, NULL);
> +
> + klist_add_head(&a, &mylist);
> + klist_add_head(&b, &mylist);
> + klist_add_before(&c, &a);
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + klist_add_before(&d, &b);
> + KUNIT_EXPECT_EQ(test, node_count, 4);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &d);
> +
> + klist_iter_init(&mylist, &i);
> +
> + /* should be [list] -> b -> d -> a -> c*/
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &c);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +/* Verify that klist_del() delays the deletion of a node until there
> + * are no other references to it
> + */
These comments are in the wrong style:
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
They should be of the form:
/*
* Text
* More text
*/
(with the empty line at the top)
> +static void klist_test_del_refcount_greater_than_zero(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, &check_delete_node);
> +
> + /* Add nodes a,b,c,d to the list*/
> + klist_add_tail(&a, &mylist);
> + klist_add_tail(&b, &mylist);
> + klist_add_tail(&c, &mylist);
> + klist_add_tail(&d, &mylist);
> +
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + /* Advance the iterator to point to node c*/
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &c);
> +
> + /* Try to delete node c while there is a reference to it*/
> + klist_del(&c);
> +
> + /*
> + * Verify that node c is still attached to the list even after being
> + * deleted. Since the iterator still points to c, the reference count is not
> + * decreased to 0
> + */
> + KUNIT_EXPECT_TRUE(test, klist_node_attached(&c));
> +
> + /* Check that node c has not been removed yet*/
> + KUNIT_EXPECT_EQ(test, node_count, 4);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &d);
> +
> + klist_iter_exit(&i);
> +
> + /* Since the iterator is no longer pointing to node c, node c is removed
> + * from the list
> + */
Nit: comment formatting. The comment above ("Verify that node c...") looks good.
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> +}
> +
> +/* Verify that klist_del() deletes a node immediately when there are no
> + * other references to it.
> + */
As above, comment formatting.
> +static void klist_test_del_refcount_zero(struct kunit *test)
> +{
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, &check_delete_node);
> +
> + /* Add nodes a,b,c,d to the list*/
> + klist_add_tail(&a, &mylist);
> + klist_add_tail(&b, &mylist);
> + klist_add_tail(&c, &mylist);
> + klist_add_tail(&d, &mylist);
> + /* Delete node c*/
> + klist_del(&c);
> +
> + /* Check that node c is deleted from the list*/
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + /* Should be [list] -> a -> b -> d*/
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_remove(struct kunit *test)
> +{
> + /* This test doesn't check correctness under concurrent access */
> + struct klist_node a, b, c, d;
> + struct klist mylist;
> + struct klist_iter i;
> +
> + node_count = 0;
> + klist_init(&mylist, &check_node, &check_delete_node);
> +
> + /* Add nodes a,b,c,d to the list*/
> + klist_add_tail(&a, &mylist);
> + klist_add_tail(&b, &mylist);
> + klist_add_tail(&c, &mylist);
> + klist_add_tail(&d, &mylist);
> + /* Delete node c*/
> + klist_remove(&c);
> +
> + /* Check the nodes in the list*/
> + KUNIT_EXPECT_EQ(test, node_count, 3);
> + KUNIT_EXPECT_PTR_EQ(test, last_node_count, &c);
> +
> + /* should be [list] -> a -> b -> d*/
> + klist_iter_init(&mylist, &i);
> +
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &a);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &b);
> + KUNIT_EXPECT_PTR_EQ(test, klist_next(&i), &d);
> + KUNIT_EXPECT_NULL(test, klist_next(&i));
> +
> + klist_iter_exit(&i);
> +
> +}
> +
> +static void klist_test_node_attached(struct kunit *test)
> +{
> + struct klist_node a = {};
> + struct klist mylist;
> +
> + klist_init(&mylist, NULL, NULL);
> +
> + KUNIT_EXPECT_FALSE(test, klist_node_attached(&a));
> + klist_add_head(&a, &mylist);
> + KUNIT_EXPECT_TRUE(test, klist_node_attached(&a));
> + klist_del(&a);
> + KUNIT_EXPECT_FALSE(test, klist_node_attached(&a));
> +
> +}
> +
> +static struct kunit_case klist_test_cases[] = {
> + KUNIT_CASE(klist_test_add_tail),
> + KUNIT_CASE(klist_test_add_head),
> + KUNIT_CASE(klist_test_add_behind),
> + KUNIT_CASE(klist_test_add_before),
> + KUNIT_CASE(klist_test_del_refcount_greater_than_zero),
> + KUNIT_CASE(klist_test_del_refcount_zero),
> + KUNIT_CASE(klist_test_remove),
> + KUNIT_CASE(klist_test_node_attached),
> + {},
> +};
> +
> +static struct kunit_suite klist_test_module = {
> + .name = "klist",
> + .test_cases = klist_test_cases,
> +};
> +
> +kunit_test_suites(&list_test_module, &hlist_test_module, &klist_test_module);
>
> MODULE_LICENSE("GPL v2");
> --
> 2.40.0.348.gf938b09366-goog
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)
Powered by blists - more mailing lists