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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c424cb6-3c2e-47a2-aa75-98fb20d805c9@linux.ibm.com>
Date: Thu, 20 Feb 2025 09:49:04 -0600
From: Nick Child <nnac123@...ux.ibm.com>
To: Dave Marquardt <davemarq@...ux.ibm.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, horms@...nel.org,
        david.laight.linux@...il.com, nick.child@....com, pmladek@...e.com,
        rostedt@...dmis.org, john.ogness@...utronix.de,
        senozhatsky@...omium.org
Subject: Re: [PATCH net-next v3 2/3] hexdump: Use for_each macro in
 print_hex_dump

Hi Dave,

Thanks for reviewing.

On 2/19/25 3:54 PM, Dave Marquardt wrote:
> Nick Child <nnac123@...ux.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