linux-kernel - Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210614064205.GA29220@lst.de>
Date:   Mon, 14 Jun 2021 08:42:05 +0200
From:   Christoph Hellwig <hch@....de>
To:     André Almeida <andrealmeid@...labora.com>
Cc:     Christoph Hellwig <hch@....de>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        Brendan Higgins <brendanhiggins@...gle.com>,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        Shuah Khan <shuah@...nel.org>, ~lkcamp/patches@...ts.sr.ht,
        nfraprado@...labora.com, leandro.ribeiro@...labora.com,
        Vitor Massaru Iha <vitor@...saru.org>, lucmaga@...il.com,
        David Gow <davidgow@...gle.com>,
        Daniel Latypov <dlatypov@...gle.com>, tales.aparecida@...il.com
Subject: Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit

> +config UUID_KUNIT_TEST
> +	tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
> +	depends on KUNIT
> +	default KUNIT_ALL_TESTS
> +	help
> +	  This builds the UUID unit test.

Does this first help line really add any value if we have this second
line:

> +	  Tests parsing functions for UUID/GUID strings.

?

> +	  If unsure, say N.

Not specific to this case, but IMHO we can drop this line for all kunit
tests as it is completely obvious.

> @@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
>  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
>  obj-$(CONFIG_BITS_TEST) += test_bits.o
>  obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> +obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o

Another meta-comment on the kunit tests:  Wouldn't it make more sense
to name them all as CONFIG_KUNIT_TEST_FOO to allow for easier grepping?

> -struct test_uuid_data {
> +struct test_data {
>  	const char *uuid;
>  	guid_t le;
>  	uuid_t be;
>  };
>  
> -static const struct test_uuid_data test_uuid_test_data[] = {
> +static const struct test_data correct_data[] = {

What is the reason for these renames?  Is this a pattern used for
other kunit tests?

> +static void uuid_correct_le(struct kunit *test)
>  {
> +	guid_t le;
> +	const struct test_data *data = (const struct test_data *)(test->param_value);

Overly long line.  But as far as I can tell there is no need for the
case that causes this mess anyway given that param_value is a
"const void *".

Same for all the other instances of this.

> +static void uuid_wrong_le(struct kunit *test)
>  {
>  	guid_t le;
> +	const char **data = (const char **)(test->param_value);

No need for the second pair of braces.  Same for various other instances.