On Mon, Dec 30, 2024 at 12:35:57PM +0100, Miquel Raynal wrote:
> On 24/12/2024 at 17:49:25 +02, Andy Shevchenko <andriy.shevchenko@linux.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@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.
>
> 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*.
Why? I mean the prefixes are fine, but the main part should be the same as long
as we use the same backend API, no? This is a lib/ code which should be suitable
for many cases, including ABI, I don't see the good reason why we should
deviate from that.
> 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.
I disagree on this. When we touch printf() stuff, we consider all aspects of
the formatted strings, and not only kernel buffer for some features. One may
use memory buffer for that, one may rely on kernel message.
> > 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.
If it becomes a part of ABI, why not? seq_printf() also uses limited buffer and
may become quite untrivial to handle when it's bigger (see all those special
cases when seq options are defined to custom callbacks).
> 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.
I think it has a relation. The use of hex_dump_to_buffer() is often to get list
of lines at the end to be formatted (and printed if required), it usually makes
not much sense for a single line as it's easier to open code and rarely
people would seek for an ASCII representation part for a single line.
> Plus, what you ask for would
> require a new buffer to be allocated, potentially big and unbounded,
I don't think you need it. What you need is a crc32 of the content or another hash.
It will be a new function that takes some kind of context, one field of which is
the hash (of the previous data).
> 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.
Do you want me to take over and show how it should be done? Definitely I can
invest some time into it, but I can't guarantee the quick result though.
> 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?
People (maintainers) who care about lib/ contents.
Mainly I implied Rasmus, but sometimes others also want to see test cases.
> There is no test case checking the kernel buffer printing.
After looking around it seems you are right.
> 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.
I understand your context here, the problem is that this is universal API.
> > (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.
P.S. You probably want to acquire somebody's else tag from PRINTK/PRINTF
maintainers/reviewers pool.
--
With Best Regards,
Andy Shevchenko