Message-ID: <87ed1p2yjm.fsf@bootlin.com>
Date: Mon, 30 Dec 2024 12:35:57 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: Petr Mladek <pmladek@...e.com>,  Steven Rostedt <rostedt@...dmis.org>,
  Rasmus Villemoes <linux@...musvillemoes.dk>,  Sergey Senozhatsky
 <senozhatsky@...omium.org>,  Jonathan Corbet <corbet@....net>,  John
 Ogness <john.ogness@...utronix.de>,  Andrew Morton
 <akpm@...ux-foundation.org>,  Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>,  linux-doc@...r.kernel.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] hexdump: Allow skipping identical lines

Andy,

On 24/12/2024 at 17:49:25 +02, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:

> 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@...ux.intel.com> wrote:
>> > On Tue, Aug 27, 2024 at 11:01:47AM +0200, Miquel Raynal wrote:
>> >> andriy.shevchenko@...ux.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.

No, it's always been like that. The formatting of the prefixes have
always been totally different in this function, probably because filling
a memory buffer and sending these messages to dmesg is fundamentally
different and we may want a different behaviour *when sending to the
kernel buffer*. That is what I believe is useful. If someone wishes to
port the feature to the other functions, they can, but it is irrelevant
to the change brought here.

> Moreover it lefts seq_hex_dump() out of the picture.

seq_hex_dump() has diverged already and is a very specific case that is
probably under stability constraints, where trimming down the output
lines is likely much less useful.

I am not strongly opposed to it, but it is probably a bit useless.

> 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.

No... that is not how these function work nor have been designed for,
hex_dump_to_buffer is used as a line-oriented helper to fill a kernel
buffer line which *is* a line-oriented buffer. A prefix gets added to
it, and I want to skip the printing if it's redundant. Lines are sent
one after the other using printk anyway. All this logic has nothing to
do inside hex_dump_to_buffer at all. Plus, what you ask for would
require a new buffer to be allocated, potentially big and unbounded,
which would require a GFP flag (yay! a new parameter). And finally the
implementation itself would be much less obvious if the algorithm was
not line oriented.

I am sorry, but I really took the time to understand your request but it
is simply not working well technically nor conceptually desirable IMO.

>> 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.

Please Andy, stop asking loads and loads of unachievable changes to fit
your ideal. I am bringing something that I feel can be useful, just stop
with your endless list of unrelated (and in this case unreachable)
wishes. There is nothing testing the prefixes, if there was a suitable
test case I would improve it, but there is none, because the test suite
is not used like that.

> And it does not matter if it's only for debugging or for ABI, we require test
> cases for the lib/ changes.

Who's we? Where is this documented?
There is no test case checking the kernel buffer printing.
I'm not skilled enough to write one.

> We don't know and don't care much about how these
> new features will be utilized

And this is typically what I dislike in some of your reviews. There is
always a context, trying to ignore it is pointless.

> (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).

I know this is generally requested, but once again, dumping pages of
data where this flag might be useful makes sense during debug sessions
in very particular places, keeping these debug calls does not seem very
useful to me, but that's certainly easier to achieve than your previous
request.

Miquèl

Powered by blists - more mailing lists