Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220802161206.228707-1-mairacanal@riseup.net>
 <CAGS_qxoAc934AwB7SZ34PpoVxvF3Eua-g+aO77kjJf6d1m0+Qw@mail.gmail.com> <2a0dcd75-5461-5266-2749-808f638f4c50@riseup.net>
In-Reply-To: <2a0dcd75-5461-5266-2749-808f638f4c50@riseup.net>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Tue, 2 Aug 2022 12:36:16 -0700
Message-ID: <CAGS_qxpYYC+k7b+-txWxEgE-VTwCF5R+WVf=qwbf3yFurcLK3w@mail.gmail.com>
Subject: Re: [PATCH 0/3] Introduce KUNIT_EXPECT_ARREQ and KUNIT_EXPECT_ARRNEQ macros
To:     =?UTF-8?B?TWHDrXJhIENhbmFs?= <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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 2, 2022 at 11:43 AM Ma=C3=ADra Canal <mairacanal@riseup.net> wr=
ote:
> > But perhaps we could instead highlight the bad bytes with something lik=
e
> > dst =3D=3D
> > 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 =3D=3D
> > 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 =3D alloc_string_stream(test, GFP_KER=
NEL);
        struct string_stream *stream2 =3D alloc_string_stream(test, GFP_KER=
NEL);

        for (i =3D 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] =3D=3D 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[] =3D {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xde,
0xad, 0xbe, 0xef};
        const u8 a2[] =3D {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.