Andy,
On 24/12/2024 at 17:49:25 +02, Andy Shevchenko [off-list ref] wrote:
On Tue, Dec 24, 2024 at 12:56:26PM +0100, Miquel Raynal wrote:
quoted
On 27/08/2024 at 16:29:18 +03, Andy Shevchenko [off-list ref] wrote:
quoted
On Tue, Aug 27, 2024 at 11:01:47AM +0200, Miquel Raynal wrote:
quoted
andriy.shevchenko@linux.intel.com wrote on Mon, 26 Aug 2024 20:32:20
+0300:
quoted
On Mon, Aug 26, 2024 at 06:24:14PM +0200, Miquel Raynal wrote:
...
quoted
quoted
quoted
quoted
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.
quoted
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.)
quoted
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
On Mon, Dec 30, 2024 at 12:35:57PM +0100, Miquel Raynal wrote:
On 24/12/2024 at 17:49:25 +02, Andy Shevchenko [off-list ref] wrote:
quoted
On Tue, Dec 24, 2024 at 12:56:26PM +0100, Miquel Raynal wrote:
quoted
On 27/08/2024 at 16:29:18 +03, Andy Shevchenko [off-list ref] wrote:
quoted
On Tue, Aug 27, 2024 at 11:01:47AM +0200, Miquel Raynal wrote:
quoted
andriy.shevchenko@linux.intel.com wrote on Mon, 26 Aug 2024 20:32:20
+0300:
quoted
On Mon, Aug 26, 2024 at 06:24:14PM +0200, Miquel Raynal wrote:
...
quoted
quoted
quoted
quoted
quoted
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.
quoted
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.
quoted
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.
quoted
quoted
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.)
quoted
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.
quoted
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.
quoted
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.
quoted
(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