linux-kernel - Re: [PATCH v3 2/2] lib/btree.c: add testcase for in-memory b+ tree

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 18 May 2017 13:00:47 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Leno Hou <lenohou@...il.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v3 2/2] lib/btree.c: add testcase for in-memory b+ tree

On Thu, May 18, 2017 at 11:35 AM, Leno Hou <lenohou@...il.com> wrote:

Commit message, please.

And I think you do introduce test cases first, then you update the
code. This way is naturally like things would be done.

> +++ b/lib/btree_test.c

Okay, this will be 5th in the pattern foo_bar_test.c :-)
I think it's okay since we have that pattern for tree implementations.

> @@ -0,0 +1,77 @@
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/btree.h>
> +#include <linux/types.h>

Alphabetical order, please.

> +
> +#define NODES 24

BTREE_TEST_...

> +
> +struct test_node {
> +       u32 key;
> +       u32 val;
> +};
> +
> +static struct btree_head32 bh;
> +static struct test_node nodes[NODES];
> +
> +static void init(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < NODES; i++) {
> +               nodes[i].key = i;
> +               nodes[i].val = i;
> +       }
> +}
> +static int __init btree_test_init(void)
> +{

> +       u32 key = 0;
> +       u32 *val = NULL;

Assignments look to me redundant. for_each should do that, shouldn't it?

> +       int i, rc;

unsigned int i;
?

> +

> +       pr_alert("btree testing\n");

We are using pr_info() / pr_warn() pair in the rest of test_* modules.
Please, follow.
E.g. here pr_info() is suitable.

> +
> +       init();

> +       rc = btree_init32(&bh);
> +

Move this empty line to be after init();.

> +       if (rc)
> +               pr_alert("Unable initialize btree memory\n");
> +
> +       for (i = 0; i < NODES; i++) {
> +               rc = btree_insert32(&bh,
> +                               nodes[i].key,
> +                               &nodes[i].val,

> +                               GFP_ATOMIC);

Is this necessary to be ATOMIC?

> +
> +               if (rc)
> +                       pr_alert("Unable to insert key into btree\n");
> +       }
> +
> +       pr_alert("========================================\n");
> +
> +       btree_for_each_safe32(&bh, key, val) {
> +               pr_alert("val %d\n", *val);
> +       }
> +
> +       btree_remove32(&bh, 11);
> +
> +       pr_alert("========================================\n");
> +       btree_for_each_safe32(&bh, key, val) {
> +               pr_alert("val %d\n", *val);
> +       }
> +

> +       return 0;

Consider to return -EINVAL if tests are failed. Thus, you need to
introduce actual testing function.

> +}

-- 
With Best Regards,
Andy Shevchenko