Re: [PATCH 0/2] hexdump: Allow skipping identical lines

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Dec 24, 2024 at 12:56:26PM +0100, Miquel Raynal wrote:
> On 27/08/2024 at 16:29:18 +03, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Aug 27, 2024 at 11:01:47AM +0200, Miquel Raynal wrote:
> >> andriy.shevchenko@linux.intel.com wrote on Mon, 26 Aug 2024 20:32:20
> >> +0300:
> >> > On Mon, Aug 26, 2024 at 06:24:14PM +0200, Miquel Raynal wrote:

...

> >> > Also here is the formal NAK till the series gains the test cases.
> >> 
> >> What test cases are you talking about?
> >
> > Anything meaningful you come up with to show that the printed data is
> > what it's expected. The module has a complimentary test case,
> > lib/test_hexdump.c. Without changes in that file, there is no go
> > to what ever golden ideas you have.
> 
> I had a look. The tests never test the content of the kernel buffer,
> while this is the only part that my changes have an impact on.

So, it means that after your change there will be a deviation between the core
function that dumps into a buffer and one that prints message into the kernel
buffer. Moreover it lefts seq_hex_dump() out of the picture.

I think you need to start from modifying hex_dump_to_buffer() to have a
functionality you want (note, there are cases in the kernel that use
hex_dump_to_buffer() for formatting messages in the kernel buffer and they
might want to have the same functionality to be available.

> These tests verify the hex_dump_to_buffer() logic, but never how it is used
> through the print_hex_dump_*() helpers.

I haven't checked and don't remember for sure, but KUnit rings a bell that it
might be possible to test the actual kernel output. (However, after the above
modifications been made it won't be needed anymore as test_hexdump.c will be
extended to support new feature.)

> In this series I am just enabling a new way to print the content of the
> buffer, like for instance enabling a prefix, which is not directly
> related to the core implementation of hexdump.
> 
> Hence, I am sorry, but I will disregard this request unless someone
> comes up with a working idea which is worth the effort, considering the
> minimum impact of this change and the fact that it is mostly (if not
> only) for debugging purposes and will most likely never reach users.

I'm sorry, but my NAK still stands. No tests — no go.

And it does not matter if it's only for debugging or for ABI, we require test
cases for the lib/ changes. We don't know and don't care much about how these
new features will be utilized (the requirement here is to have a user for it,
so you might need to consider to convert one of the existing user to use a new
feature, besides the [updated] test cases).

-- 
With Best Regards,
Andy Shevchenko