On Tue, Aug 2, 2022 at 11:43 AM Maíra Canal <mairacanal@xxxxxxxxxx> 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.
