Re: [PATCH 0/3] Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros

From: Daniel Latypov <dlatypov@google.com>
To: "Maíra Canal" <mairacanal@riseup.net>
Cc: Brendan Higgins <brendanhiggins@google.com>,
	davidgow@google.com, airlied@linux.ie, daniel@ffwll.ch,
	davem@davemloft.net, kuba@kernel.org, jose.exposito89@gmail.com,
	javierm@redhat.com, andrealmeid@riseup.net,
	melissa.srw@gmail.com, siqueirajordao@riseup.net,
	Isabella Basso <isabbasso@riseup.net>,
	magalilemes00@gmail.com, tales.aparecida@gmail.com,
	linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/3] Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros
Date: Tue, 2 Aug 2022 12:36:16 -0700	[thread overview]
Message-ID: <CAGS_qxpYYC+k7b+-txWxEgE-VTwCF5R+WVf=qwbf3yFurcLK3w@mail.gmail.com> (raw)
In-Reply-To: <2a0dcd75-5461-5266-2749-808f638f4c50@riseup.net>

On Tue, Aug 2, 2022 at 11:43 AM Maíra Canal <mairacanal@riseup.net> wrote:
> > But perhaps we could instead highlight the bad bytes with something like
> > dst ==
> > 00000000: 33 0a 60 12 00 a8 00 00 00 00 <8e> 6b 33 0a 60 12
> > 00000010: 00 00 00 00 <00> a8 8e 6b 33 0a 00 00 00 00
> > result->expected ==
> > 00000000: 31 0a 60 12 00 a8 00 00 00 00 <81> 6b 33 0a 60 12
> > 00000010: 00 00 00 00 <01> a8 8e 6b 33 0a 00 00 00 00
>
> My problem with this approach is that the bytes get slightly misaligned
> when adding the <>. Maybe if we aligned as:
>
> dst:
> 00000000: <33> 0a 60 12  00  a8 00 00 00 00 <8e> 6b 33 0a 60 12
> 00000010:  00  00 00 00 <00> a8 8e 6b 33 0a  00  00 00 00
> result->expected:
> 00000000: <31> 0a 60 12  00  a8 00 00 00 00 <81> 6b 33 0a 60 12
> 00000010:  00  00 00 00 <01> a8 8e 6b 33 0a  00  00 00 00

And yes, that's a good point re alignment. Handling that would be
annoying and perhaps a reason to leave this off until later.

Perhaps in the short-term, we could add output like
  First differing byte at index 0
if others think that could be useful.

I'm quite surprised I didn't notice the first bytes differed (as you
can tell from my example), so I personally would have been helped out
by such a thing.

>
> Although I don't know exactly how we can produce this output. I was
> using hex_dump_to_buffer to produce the hexdump, so maybe I need to
> change the strategy to generate the hexdump.

Indeed, we'd probably have to write our own code to do this.
I think it might be reasonable to stick with the code as-is so we can
just reuse hex_dump_to_buffer.
We'd then be able to think about the format more and bikeshed without
blocking this patch.

But note: we could leverage string_stream to build up the output a bit
more easily than you might expect.
Here's a terrible first pass that you can paste into kunit-example-test.c

#include "string-stream.h"

static void diff_hex_dump(struct kunit *test, const u8 *a, const u8 *b,
                          size_t num_bytes, size_t row_size)
{
        size_t i;
        struct string_stream *stream1 = alloc_string_stream(test, GFP_KERNEL);
        struct string_stream *stream2 = alloc_string_stream(test, GFP_KERNEL);

        for (i = 0; i < num_bytes; ++i) {
                if (i % row_size) {
                        string_stream_add(stream1, " ");
                        string_stream_add(stream2, " ");
                } else {
                        string_stream_add(stream1, "\n> ");
                        string_stream_add(stream2, "\n> ");
                }

                if (a[i] == b[i]) {
                        string_stream_add(stream1, "%02x", a[i]);
                        string_stream_add(stream2, "%02x", b[i]);
                } else {
                        string_stream_add(stream1, "<%02x>", a[i]);
                        string_stream_add(stream2, "<%02x>", b[i]);
                }
        }
        string_stream_add(stream1, "\nwant");
        string_stream_append(stream1, stream2);

        kunit_info(test, "got%s\n", string_stream_get_string(stream1));
}

static void example_hex_test(struct kunit *test) {
        const u8 a1[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xde,
0xad, 0xbe, 0xef};
        const u8 a2[] = {0x1, 0x3, 0x2, 0x4, 0x5, 0x6, 0x7, 0xde,
0xad, 0xbe, 0xef};

        diff_hex_dump(test, a1, a2, sizeof(a1), 8);
}

It produces the following output:
    # example_hex_test: got
> 01 <02> <03> 04 05 06 07 de
> ad be ef
want
> 01 <03> <02> 04 05 06 07 de
> ad be ef

It doesn't handle re-aligning the other bytes as you'd pointed out above.

>
> I guess the KASAN approach could be easier to implement. But I guess it
> can turn out to be a little polluted if many bytes differ. For example:
>
> dst:
> 00000000: 33 31 31 31 31 31 31 31 31 31 8e 31 33 0a 60 12
>            ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^
> 00000010: 00 00 00 00 00 a8 8e 6b 33 0a 00 00 00 00
>                        ^
> result->expected:
> 00000000: 31 0a 60 12 00 a8 00 00 00 00 81 6b 33 0a 60 12
>            ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^  ^
> 00000010: 00 00 00 00 01 a8 8e 6b 33 0a 00 00 00 00
>                        ^
>
> I don't know exactly with option I lean.

Agreed, it doesn't scale up too well when pointing out >1 buggy bytes.