| Message ID | 20250219211102.225324-3-nnac123@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Use new for_each macro to create hexdumps | expand |
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
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 > >
On Thu, Feb 20, 2025 at 09:41:18PM +0000, David Laight wrote: > On Thu, 20 Feb 2025 09:49:04 -0600 > Nick Child <nnac123@linux.ibm.com> wrote: > > 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. > I see now, I forgot about print_hex_dump's use of for_each_line_in_hexdump in my response the morning. Yes the ascii argument needs to be respected and this patch forces false, this is an oversight on my end, thanks for pointing it out. I will address this issue in the next version by passing the ascii argument into the for_each loop. Will wait a few weeks before sending this time. - Nick
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) { const u8 *ptr = buf; - int i, linelen, remaining = len; + int i; unsigned char linebuf[32 * 3 + 2 + 32 + 1]; if (rowsize != 16 && rowsize != 32) rowsize = 16; - 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) { switch (prefix_type) { case DUMP_PREFIX_ADDRESS: printk("%s%s%p: %s\n",
The looping logic in print_hex_dump can be handled by the macro for_each_line_in_hex_dump. Signed-off-by: Nick Child <nnac123@linux.ibm.com> --- lib/hexdump.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)