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

[David Laight <david.laight.linux@gmail.com>]

On Fri, 21 Feb 2025 11:37:46 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> 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 <nnac123@linux.ibm.com> wrote:
> >   
> > > ---
> > >  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.

> 
> > > +	     (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, \
          ^ needs to be buf_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?

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.
> 
> > 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/