linux-kernel - Re: [PATCH v2 4/9] drm: selftest: convert drm

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]

Message-ID: <CAGS_qxrynX=q7ZVmFSm-HO-3r3wGVnXMXVMejM3-ONvyJUrrPg@mail.gmail.com>
Date:   Wed, 22 Jun 2022 10:06:42 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Maíra Canal <maira.canal@....br>
Cc:     Isabella Basso <isabbasso@...eup.net>, magalilemes00@...il.com,
        tales.aparecida@...il.com, mwen@...lia.com, andrealmeid@...eup.net,
        siqueirajordao@...eup.net, Trevor Woerner <twoerner@...il.com>,
        leandro.ribeiro@...labora.com, n@...aprado.net,
        Daniel Vetter <daniel@...ll.ch>,
        Shuah Khan <skhan@...uxfoundation.org>,
        David Airlie <airlied@...ux.ie>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        michal.winiarski@...el.com,
        Javier Martinez Canillas <javierm@...hat.com>,
        José Expósito <jose.exposito89@...il.com>,
        David Gow <davidgow@...gle.com>, brendanhiggins@...gle.com,
        kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/9] drm: selftest: convert drm_format selftest to KUnit

On Tue, Jun 21, 2022 at 1:10 PM Maíra Canal <maira.canal@....br> wrote:
>
> Considering the current adoption of the KUnit framework, convert the
> DRM format selftest to the KUnit API.
>
> Tested-by: David Gow <davidgow@...gle.com>
> Signed-off-by: Maíra Canal <maira.canal@....br>

Acked-by: Daniel Latypov <dlatypov@...gle.com>

Overall looks good from the KUnit side, just a few general suggestions below.

FYI, the warning email from kernel-test-robot is basically saying that
the compiler is not optimizing away the temporary variables internally
created in KUNIT_EXPECT_*.
So having too many KUNIT_EXPECT_.* in a single function is the trigger.
The main workaround you'd have is to split up the test into more test functions.
(I don't know if that's actually worth doing)

> +static void igt_check_drm_format_block_width(struct kunit *test)
> +{
> +       const struct drm_format_info *info = NULL;
> +
> +       /* Test invalid arguments */
> +       KUNIT_EXPECT_FALSE(test, drm_format_info_block_width(info, 0));
> +       KUNIT_EXPECT_FALSE(test, drm_format_info_block_width(info, -1));
> +       KUNIT_EXPECT_FALSE(test, drm_format_info_block_width(info, 1));

Hmm, I think one of these two would be clearer here:
KUNIT_EXPECT_EQ(test, drm_format_info_block_width(info, 0), 0);
KUNIT_EXPECT_EQ(test, 0, drm_format_info_block_width(info, 0));

I think this helps test readability (giving hints about the types) and
gives better error messages, more on that below.

The problem with using the boolean expectations is that given
  int foo = 2;
  KUNIT_EXPECT_FALSE(test, foo);
KUnit will only print out
    Expected foo to be false, but is true

Using EXPECT_EQ(foo, 0), we'd get
    Expected foo == 0, but
        foo == 2

Knowing exactly what the offending return value was can help debug
test failures a bit faster.

> +
> +       /* Test 1 plane format */
> +       info = drm_format_info(DRM_FORMAT_XRGB4444);
> +       KUNIT_EXPECT_TRUE(test, info);

FYI, you can now instead write
  KUNIT_EXPECT_NOT_NULL(test, info);
this new macro was merged into 5.19-rc1.