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

On Fri, 21 Feb 2025 11:37:46 -0600
Nick Child [off-list ref] wrote:

Hi David,

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

quoted

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.

There are plenty of ways to generate infinite loops.
I wouldn't worry about someone passing 0 for rowsize.

quoted

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.

quoted

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, \

          ^ needs to be buf_offset.

quoted

		_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?

No, it is to prevent side-effects happening more than once.
Think about what would happen if someone passed 'foo -= 4' for len.

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.

The c std level got upped recently to allow declarations inside loops.
Usually for a 'loop iterator' - but I think you needed that to be
exposed outsize the loop.
(Otherwise you don't need _offset and buf_offset.

I suppose both implementations come with tradeoffs.

quoted

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.

My changed version (honoring row_size) passes all the self tests.
I've not done a grep (yet) to if anything other that 16 or 32 is
actually passed.

	David

Appreciate the review!
- Nick

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