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.