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

[Nick Child <nnac123@linux.ibm.com>]

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.


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.


- Nick