Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in print_hex_dump

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

On Thu, 20 Feb 2025 09:49:04 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> Hi Dave,
> 
> Thanks for reviewing.
> 
> On 2/19/25 3:54 PM, Dave Marquardt wrote:
> > Nick Child <nnac123@linux.ibm.com> writes:
> >  
> >> diff --git a/lib/hexdump.c b/lib/hexdump.c
> >> index c3db7c3a7643..181b82dfe40d 100644
> >> --- a/lib/hexdump.c
> >> +++ b/lib/hexdump.c
> >> @@ -263,19 +263,14 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >>   		    const void *buf, size_t len, bool ascii)
> >>   {
> >> -	for (i = 0; i < len; i += rowsize) {
> >> -		linelen = min(remaining, rowsize);
> >> -		remaining -= rowsize;
> >> -
> >> -		hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
> >> -				   linebuf, sizeof(linebuf), ascii);
> >> -
> >> +	for_each_line_in_hex_dump(i, rowsize, linebuf, sizeof(linebuf),
> >> +				  groupsize, buf, len) {  
> > Several callers of print_hex_dump pass true for the ascii parameter,
> > which gets passed along to hex_dump_to_buffer. But you ignore it in
> > for_each_line_in_hex_dump and always use false:
> >
> > + #define for_each_line_in_hex_dump(i, rowsize, linebuf, linebuflen, groupsize, \
> > +				   buf, len) \
> > +	for ((i) = 0;							\
> > +	     (i) < (len) &&						\
> > +	     hex_dump_to_buffer((unsigned char *)(buf) + (i),		\
> > +				(len) - (i), (rowsize), (groupsize),	\
> > +				(linebuf), (linebuflen), false);	\
> > +	     (i) += (rowsize) == 32 ? 32 : 16				\
> > +	    )
> >
> > Is this behavior change intended?
> >
> > -Dave  
> 
> Yes, for simplicity, I wanted to limit the number of parameters that the 
> macro had.
> 
> Since the function does not do any printing, the user can do ascii 
> conversion on their own
> 
> or even easier just ensure a \NULL term and print the string with the %s 
> format specifier.

That just isn't the same.
The hexdump code 'sanitises' the string for you.

> 
> 
> Also, allowing the user to specify the ascii argument makes it more 
> difficult for them to calculate the correct linebuflen.
> 
> For example, rowlen == 16, and groupsize == 1 with ascii = true would 
> require a linebuflen
> 
> of 16 * 4 + 1 (2 chars, 1 space/NULL and 1 ascii per byte  + a extra 
> space separating hexdump and ascii).
> 
> If ascii == false, linebuflen is very logically 16*3.

But the buffer only need to be 'big enough', a few spare bytes don't
matter.

You can't change the behaviour like that.

	David

> 
> 
> - Nick
> 
>