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

Date:   Mon, 14 Jun 2021 18:08:37 -0300
From:   André Almeida <andrealmeid@...labora.com>
To:     Daniel Latypov <dlatypov@...gle.com>,
        Christoph Hellwig <hch@....de>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        KUnit Development <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>, tales.aparecida@...il.com
Subject: Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit

Às 13:55 de 14/06/21, Daniel Latypov escreveu:
> On Sun, Jun 13, 2021 at 11:42 PM Christoph Hellwig <hch@....de> wrote:
>>
>>> +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?
> 
> But putting them in a "kunit namespace" by prefixing them as such
> would be misleading, IMO.
> The tests live adjacent to the code they test and are owned by the
> same maintainers, or at least that's the intent.
> 
> And if the goal is just to find configs, then I don't see much
> difference between "config.*KUNIT_TEST" and "config KUNIT_TEST.*"
> 
>>
>>> -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?
> 
> No, this is not a pattern.
> The structs can be renamed back.
> 

The idea behind this renaming is to be more explicit about what this
data is about: correct UUIDs inputs.

>>
>>> +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 *".
> 
> There is no need for the cast or the brace, yes.
> This is my fault.
> 
> The documentation has both since I had thought that would make how it
> works more clear:
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
> I don't really understand my past thought process...
> 

Ok, I'll change my code to remove the cast and braces. I can also send a
patch to rework this part of documentation.

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