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