| 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: <CAGS_qxr2yZmcZRGF3qg9eSqOAwCj3+CD4Gyc8cgTXydRToDckw@mail.gmail.com>
Date: Tue, 27 Jul 2021 17:04:08 -0700
From: Daniel Latypov <dlatypov@...gle.com>
To: José Aquiles Guedes de Rezende
<jjoseaquiless@...il.com>
Cc: Jiri Pirko <jiri@...dia.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, brendanhiggins@...gle.com,
davidgow@...gle.com, linux-kselftest@...r.kernel.org,
~lkcamp/patches@...ts.sr.ht,
Matheus Henrique de Souza Silva
<matheushenriquedesouzasilva@...tonmail.com>
Subject: Re: [PATCH] lib: use of kunit in test_parman.c
On Tue, Jul 27, 2021 at 4:04 PM José Aquiles Guedes de Rezende
<jjoseaquiless@...il.com> wrote:
>
> Convert the parman test module to use the KUnit test framework.
> This makes thetest clearer by leveraging KUnit's assertion macros
nit: s/thetest/the test
> and test case definitions,as well as helps standardize on a testing framework.
>
> Co-developed-by: Matheus Henrique de Souza Silva <matheushenriquedesouzasilva@...tonmail.com>
> Signed-off-by: Matheus Henrique de Souza Silva <matheushenriquedesouzasilva@...tonmail.com>
> Signed-off-by: José Aquiles Guedes de Rezende <jjoseaquiless@...il.com>
Acked-by: Daniel Latypov <dlatypov@...gle.com>
I just briefly looked over the usage of KUnit a bit and left some suggestions.
It's nice to see the use of current->kunit_test in an ops struct.
I wrote that up as a potential use case in commit 359a376081d4
("kunit: support failure from dynamic analysis tools"), and I think
this is the first example of it being used as such :)
> ---
> lib/test_parman.c | 145 +++++++++++++++++++---------------------------
> 1 file changed, 60 insertions(+), 85 deletions(-)
>
> diff --git a/lib/test_parman.c b/lib/test_parman.c
> index 35e32243693c..bd5010f0a412 100644
> --- a/lib/test_parman.c
> +++ b/lib/test_parman.c
> @@ -41,6 +41,8 @@
> #include <linux/err.h>
> #include <linux/random.h>
> #include <linux/parman.h>
> +#include <linux/sched.h>
> +#include <kunit/test.h>
>
> #define TEST_PARMAN_PRIO_SHIFT 7 /* defines number of prios for testing */
> #define TEST_PARMAN_PRIO_COUNT BIT(TEST_PARMAN_PRIO_SHIFT)
> @@ -91,12 +93,14 @@ struct test_parman {
>
> static int test_parman_resize(void *priv, unsigned long new_count)
> {
> + struct kunit *test = current->kunit_test;
> struct test_parman *test_parman = priv;
> struct test_parman_item **prio_array;
> unsigned long old_count;
>
> prio_array = krealloc(test_parman->prio_array,
> ITEM_PTRS_SIZE(new_count), GFP_KERNEL);
> + KUNIT_EXPECT_NOT_ERR_OR_NULL(test, prio_array);
> if (new_count == 0)
> return 0;
> if (!prio_array)
> @@ -214,42 +218,39 @@ static void test_parman_items_fini(struct test_parman *test_parman)
> }
> }
>
> -static struct test_parman *test_parman_create(const struct parman_ops *ops)
> +static int test_parman_create(struct kunit *test)
> {
> struct test_parman *test_parman;
> int err;
>
> - test_parman = kzalloc(sizeof(*test_parman), GFP_KERNEL);
> - if (!test_parman)
> - return ERR_PTR(-ENOMEM);
> + test_parman = kunit_kzalloc(test, sizeof(*test_parman), GFP_KERNEL);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_parman);
> +
> err = test_parman_resize(test_parman, TEST_PARMAN_BASE_COUNT);
Would it be clearer to do
KUNIT_ASSERT_EQ(test, 0, test_parman_resize(test_parman,
TEST_PARMAN_BASE_COUNT));
or change the line below to:
KUNIT_ASSERT_EQ_MSG(test, err, 0, "test_parman_resize failed");
Otherwise, if the test fails there, the error message isn't too clear.
> - if (err)
> - goto err_resize;
> - test_parman->parman = parman_create(ops, test_parman);
> - if (!test_parman->parman) {
> - err = -ENOMEM;
> - goto err_parman_create;
> - }
> + KUNIT_ASSERT_EQ(test, err, 0);
> +
> + test_parman->parman = parman_create(&test_parman_lsort_ops, test_parman);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, test_parman->parman);
Hmm, this won't call `test_parman_resize(test_parman, 0)` on error
like it did before.
Unfortunately, KUnit is a bit clunky for cases like this right now,
where there's cleanups that need to happen but aren't handle already
by the resource API (the underlying thing behind kunit_kzalloc that
frees the mem).
We could do something like
if (IS_ERR_OR_NULL(test_parman->parman)) {
// we can use KUNIT_FAIL to just directly fail the test, or use the
assert macro for the autogenerated failure message
KUNIT_FAIL(test, "....") / KUNIT_EXPECT_NOT_ERR_OR_NULL(test, ...);
goto err_parman_create;
}
> +
> test_parman_rnd_init(test_parman);
> test_parman_prios_init(test_parman);
> test_parman_items_init(test_parman);
> test_parman->run_budget = TEST_PARMAN_RUN_BUDGET;
> - return test_parman;
> -
> -err_parman_create:
> - test_parman_resize(test_parman, 0);
> -err_resize:
> - kfree(test_parman);
> - return ERR_PTR(err);
> + test->priv = test_parman;
> + return 0;
> }
>
> -static void test_parman_destroy(struct test_parman *test_parman)
> +static void test_parman_destroy(struct kunit *test)
> {
> + struct test_parman *test_parman = test->priv;
> +
> + if (!test_parman)
> + return;
> test_parman_items_fini(test_parman);
> test_parman_prios_fini(test_parman);
> parman_destroy(test_parman->parman);
> test_parman_resize(test_parman, 0);
> - kfree(test_parman);
> + kunit_kfree(test, test_parman);
This works as-is, but FYI, it isn't necessary as we've used
kunit_kzalloc() to allocate it above.
When the test exists, it'll automatically call this function, basically.
> }
>
> static bool test_parman_run_check_budgets(struct test_parman *test_parman)
> @@ -265,8 +266,9 @@ static bool test_parman_run_check_budgets(struct test_parman *test_parman)
> return true;
> }
>
> -static int test_parman_run(struct test_parman *test_parman)
> +static void test_parman_run(struct kunit *test)
> {
> + struct test_parman *test_parman = test->priv;
> unsigned int i = test_parman_rnd_get(test_parman);
> int err;
>
> @@ -281,8 +283,8 @@ static int test_parman_run(struct test_parman *test_parman)
> err = parman_item_add(test_parman->parman,
> &item->prio->parman_prio,
> &item->parman_item);
> - if (err)
> - return err;
> + KUNIT_ASSERT_EQ(test, err, 0);
Echoing my suggestion above, we can either do
KUNIT_ASSERT_EQ(test, 0, parman_item_add(...));
or something like
KUNIT_ASSERT_EQ_MSG(test, err, 0, "parman_item_add failed")
> +
> test_parman->prio_array[item->parman_item.index] = item;
> test_parman->used_items++;
> } else {
> @@ -294,22 +296,19 @@ static int test_parman_run(struct test_parman *test_parman)
> }
> item->used = !item->used;
> }
> - return 0;
> }
>
> -static int test_parman_check_array(struct test_parman *test_parman,
> - bool gaps_allowed)
> +static void test_parman_check_array(struct kunit *test, bool gaps_allowed)
> {
> unsigned int last_unused_items = 0;
> unsigned long last_priority = 0;
> unsigned int used_items = 0;
> int i;
> + struct test_parman *test_parman = test->priv;
>
> - if (test_parman->prio_array_limit < TEST_PARMAN_BASE_COUNT) {
> - pr_err("Array limit is lower than the base count (%lu < %lu)\n",
> - test_parman->prio_array_limit, TEST_PARMAN_BASE_COUNT);
> - return -EINVAL;
> - }
> + KUNIT_ASSERT_GE_MSG(test, test_parman->prio_array_limit, TEST_PARMAN_BASE_COUNT,
> + "Array limit is lower than the base count (%lu < %lu)\n",
> + test_parman->prio_array_limit, TEST_PARMAN_BASE_COUNT);
>
> for (i = 0; i < test_parman->prio_array_limit; i++) {
> struct test_parman_item *item = test_parman->prio_array[i];
> @@ -318,77 +317,53 @@ static int test_parman_check_array(struct test_parman *test_parman,
> last_unused_items++;
> continue;
> }
> - if (last_unused_items && !gaps_allowed) {
> - pr_err("Gap found in array even though they are forbidden\n");
> - return -EINVAL;
> - }
> +
> + KUNIT_ASSERT_FALSE_MSG(test, last_unused_items && !gaps_allowed,
> + "Gap found in array even though they are forbidden\n");
FYI, you don't need the \n for these.
KUnit will put one automatically.
Ditto for the other instances.
>
> last_unused_items = 0;
> used_items++;
>
> - if (item->prio->priority < last_priority) {
> - pr_err("Item belongs under higher priority then the last one (current: %lu, previous: %lu)\n",
> - item->prio->priority, last_priority);
> - return -EINVAL;
> - }
> - last_priority = item->prio->priority;
> + KUNIT_ASSERT_GE_MSG(test, item->prio->priority, last_priority,
> + "Item belongs under higher priority then the last one (current: %lu, previous: %lu)\n",
> + item->prio->priority, last_priority);
>
> - if (item->parman_item.index != i) {
> - pr_err("Item has different index in compare to where it actually is (%lu != %d)\n",
> - item->parman_item.index, i);
> - return -EINVAL;
> - }
> - }
> + last_priority = item->prio->priority;
>
> - if (used_items != test_parman->used_items) {
> - pr_err("Number of used items in array does not match (%u != %u)\n",
> - used_items, test_parman->used_items);
> - return -EINVAL;
> - }
> + KUNIT_ASSERT_EQ_MSG(test, item->parman_item.index, (unsigned long)i,
> + "Item has different index in compare to where it actually is (%lu != %d)\n",
> + item->parman_item.index, i);
Note: the cast to `unsigned long` here shouldn't be necessary anymore,
I don't think.
See 6e62dfa6d14f ("kunit: Do not typecheck binary assertions").
>
> - if (last_unused_items >= TEST_PARMAN_RESIZE_STEP_COUNT) {
> - pr_err("Number of unused item at the end of array is bigger than resize step (%u >= %lu)\n",
> - last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT);
> - return -EINVAL;
> }
>
> - pr_info("Priority array check successful\n");
> + KUNIT_ASSERT_EQ_MSG(test, used_items, test_parman->used_items,
> + "Number of used items in array does not match (%u != %u)\n",
> + used_items, test_parman->used_items);
>
> - return 0;
> + KUNIT_ASSERT_LT_MSG(test, (unsigned long)last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT,
> + "Number of unused item at the end of array is bigger than resize step (%u >= %lu)\n",
> + last_unused_items, TEST_PARMAN_RESIZE_STEP_COUNT);
ditto here, I think the casting can be dropped now.
> }
>
> -static int test_parman_lsort(void)
> +static void test_parman_lsort(struct kunit *test)
> {
> - struct test_parman *test_parman;
> - int err;
> -
> - test_parman = test_parman_create(&test_parman_lsort_ops);
> - if (IS_ERR(test_parman))
> - return PTR_ERR(test_parman);
> -
> - err = test_parman_run(test_parman);
> - if (err)
> - goto out;
> -
> - err = test_parman_check_array(test_parman, false);
> - if (err)
> - goto out;
> -out:
> - test_parman_destroy(test_parman);
> - return err;
> + test_parman_run(test);
> + test_parman_check_array(test, false);
> }
>
> -static int __init test_parman_init(void)
> -{
> - return test_parman_lsort();
> -}
> +static struct kunit_case parman_test_case[] = {
> + KUNIT_CASE(test_parman_lsort),
> + {}
> +};
>
> -static void __exit test_parman_exit(void)
> -{
> -}
> +static struct kunit_suite parman_test_suite = {
> + .name = "parman",
> + .init = test_parman_create,
> + .exit = test_parman_destroy,
> + .test_cases = parman_test_case,
> +};
>
> -module_init(test_parman_init);
> -module_exit(test_parman_exit);
> +kunit_test_suite(parman_test_suite);
>
> MODULE_LICENSE("Dual BSD/GPL");
> MODULE_AUTHOR("Jiri Pirko <jiri@...lanox.com>");
> --
> 2.32.0
>
Powered by blists - more mailing lists