Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

Hi David,

On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:

On Wed, 19 Feb 2025 15:11:00 -0600
Nick Child [off-list ref] wrote:

quoted

---
 include/linux/printk.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 4217a9f412b2..12e51b1cdca5 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
+				   buf, len) \
+	for ((i) = 0;							\
+	     (i) < (len) &&						\
+	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
+				(len) - (i), (rowsize), (groupsize),	\
+				(linebuf), (linebuflen), false);	\

You can avoid the compiler actually checking the function result
it you try a bit harder - see below.

This was an extra precaution against infinite loops, breaking when
hex_dump_to_buffer returns 0 when len is 0. Technically this won't happen
since we check i < len first, and increment i by at least 16 (though
your proposal removes the latter assertion). 

My other thought was to check for error case by checking if
the return value was > linebuflen. But I actually prefer the behavior
of continuing with the truncated result.

I think I prefer it how it is rather than completely ignoring it.
Open to other opinons though.

quoted

+	     (i) += (rowsize) == 32 ? 32 : 16				\
+	    )

If you are doing this as a #define you really shouldn't evaluate the
arguments more than once.
I'd also not add more code that relies on the perverse and pointless
code that enforces rowsize of 16 or 32.
Maybe document it, but there is no point changing the stride without
doing the equivalent change to the rowsize passed to hex_dump_to_buffer.

The equivalent conditonal exists in hex_dump_to_buffer so doing it
again seemed unnecessary. I understand your recent patch [1] is trying
to replace the rowsize is 16 or 32 rule with rowsize is a power of 2
and multiple of groupsize. I suppose the most straightforward and
flexible thing the for_each loop can do is to just assume rowsize is
valid.

You could do:
#define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \
		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
	_offset += _rowsize )

(Assuming I've not mistyped it.)

Trying to understand the reasoning for declaring new tmp variables;
Is this to prevent the values from changing in the body of the loop?
I tried to avoid declaring new vars in this design because I thought it
would recive pushback due to possible name collision and variable
declaration inside for loop initializer.
I suppose both implementations come with tradeoffs.

As soon as 'ascii' gets replaced by 'flags' you'll need to pass it through.

Yes, if hex_dump_to_buffer becomes a wrapper around a new function
(which accepts flag arg), I think there is an opportunity for a lot
of confusion to clear up. Old behaviour of hex_dump_to_buffer will be
respected but the underlying function will be more flexible.

Appreciate the review!
- Nick

[1] - https://lore.kernel.org/lkml/20250216201901.161781-1-david.laight.linux@gmail.com/ (local)