cifs: call print_hex_dump instead of custom implementation
Subsystems (2)
PATCH v2 1/1 cifs: call print_hex_dump instead of custom implementation
This patch converts custom dumper to use native print_hex_dump() instead. The dump will have a given label and addresses per each line which differs it from the original code.
--- fs/cifs/cifs_debug.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)
fs/cifs/cifs_debug.c +2 -19
@@ -34,27 +34,10 @@void cifs_dump_mem (char *label,void *data,int length) { -int i, j; -int *intptr = data; -char *charptr = data; -char buf[10 ], line[80 ]; -printk (KERN_DEBUG "%s: dump of %d bytes of data at 0x%p\n" , label, length, data); -for (i =0 ; i < length; i +=16 ) { - line[0 ] =0 ; -for (j =0 ; (j <4 ) && (i + j *4 < length); j++) { -sprintf (buf," %08x" , intptr[i /4 + j]); -strcat (line, buf); - } - buf[0 ] =' ' ; - buf[2 ] =0 ; -for (j =0 ; (j <16 ) && (i + j < length); j++) { - buf[1 ] =isprint (charptr[i + j]) ? charptr[i + j] :'.' ; -strcat (line, buf); - } -printk (KERN_DEBUG "%s\n" , line); - } +print_hex_dump (KERN_DEBUG , label,DUMP_PREFIX_ADDRESS ,16 ,4 , + data, length, true); }#ifdef CONFIG_CIFS_DEBUG
Signature · 1 line
2.0.0.rc2
A message that is not in the archive (sent off-list or not yet received): 1399907720-18622-1-git-send-email-andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]
Quoted 33 lines · Andy Shevchenko
This patch converts custom dumper to use native
print_hex_dump()instead. The dump will have a given label and addresses per each line which differs it from the original code.--- fs/cifs/cifs_debug.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)fs/cifs/cifs_debug.c +0
-14@@ -34,27 +34,10 @@void cifs_dump_mem (char *label,void *data,int length) { -int i, j; -int *intptr = data; -char *charptr = data; -char buf[10 ], line[80 ]; -printk (KERN_DEBUG "%s: dump of %d bytes of data at 0x%p\n" , label, length, data); -for (i =0 ; i < length; i +=16 ) { - line[0 ] =0 ; -for (j =0 ; (j <4 ) && (i + j *4 < length); j++) { -sprintf (buf," %08x" , intptr[i /4 + j]); -strcat (line, buf); - } - buf[0 ] =' ' ; - buf[2 ] =0 ; -for (j =0 ; (j <16 ) && (i + j < length); j++) {
fs/cifs/cifs_debug.c line 43
Quoted 10 lines
fs/cifs/cifs_debug.c +2
-5- buf[1 ] =isprint (charptr[i + j]) ? charptr[i + j] :'.' ; -strcat (line, buf); - } -printk (KERN_DEBUG "%s\n" , line); - } +print_hex_dump (KERN_DEBUG , label,DUMP_PREFIX_ADDRESS ,16 ,4 , + data, length, true); }#ifdef CONFIG_CIFS_DEBUG
Looks good, thanks for this. You can add my Reviewed-By.
Signature · 1 line
/ Alexander Bokovoy
Jeff Layton
Acked-byQuoted 30 lines · On Mon, 12 May 2014 18:15:20 +0300 Andy Shevchenko…
This patch converts custom dumper to use native
print_hex_dump()instead. The dump will have a given label and addresses per each line which differs it from the original code.--- fs/cifs/cifs_debug.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)fs/cifs/cifs_debug.c +0
-11@@ -34,27 +34,10 @@void cifs_dump_mem (char *label,void *data,int length) { -int i, j; -int *intptr = data; -char *charptr = data; -char buf[10 ], line[80 ]; -printk (KERN_DEBUG "%s: dump of %d bytes of data at 0x%p\n" , label, length, data); -for (i =0 ; i < length; i +=16 ) { - line[0 ] =0 ; -for (j =0 ; (j <4 ) && (i + j *4 < length); j++) { -sprintf (buf," %08x" , intptr[i /4 + j]); -strcat (line, buf); - }
fs/cifs/cifs_debug.c line 40
Quoted 10 lines
fs/cifs/cifs_debug.c +2
-8- buf[0 ] =' ' ; - buf[2 ] =0 ; -for (j =0 ; (j <16 ) && (i + j < length); j++) { - buf[1 ] =isprint (charptr[i + j]) ? charptr[i + j] :'.' ; -strcat (line, buf); - } -printk (KERN_DEBUG "%s\n" , line); - } +print_hex_dump (KERN_DEBUG , label,DUMP_PREFIX_ADDRESS ,16 ,4 , + data, length, true);
I'm not sure we want "label" as the prefix_str as it'll go on every line. Maybe just use "" in place of "label" there?
fs/cifs/cifs_debug.c line 43
Quoted 3 lines
fs/cifs/cifs_debug.c +0
-0}#ifdef CONFIG_CIFS_DEBUG
Otherwise, this looks good.
Quoted 38 lines · Jeff Layton
Quoted 35 lines · On Mon, 12 May 2014 18:15:20 +0300 Andy Shevchenko…
This patch converts custom dumper to use native
print_hex_dump()instead. The dump will have a given label and addresses per each line which differs it from the original code.Signed-off-by Andy Shevchenko <[email protected]>--- fs/cifs/cifs_debug.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)fs/cifs/cifs_debug.c +0
-16@@ -34,27 +34,10 @@void cifs_dump_mem (char *label,void *data,int length) { -int i, j; -int *intptr = data; -char *charptr = data; -char buf[10 ], line[80 ]; -printk (KERN_DEBUG "%s: dump of %d bytes of data at 0x%p\n" , label, length, data); -for (i =0 ; i < length; i +=16 ) { - line[0 ] =0 ; -for (j =0 ; (j <4 ) && (i + j *4 < length); j++) { -sprintf (buf," %08x" , intptr[i /4 + j]); -strcat (line, buf); - } - buf[0 ] =' ' ; - buf[2 ] =0 ; -for (j =0 ; (j <16 ) && (i + j < length); j++) { - buf[1 ] =isprint (charptr[i + j]) ? charptr[i + j] :'.' ; -strcat (line, buf);
Quoted 8 lines
fs/cifs/cifs_debug.c line 40
Quoted 5 lines
fs/cifs/cifs_debug.c +2
-3- } -printk (KERN_DEBUG "%s\n" , line); - } +print_hex_dump (KERN_DEBUG , label,DUMP_PREFIX_ADDRESS ,16 ,4 , + data, length, true);I'm not sure we want "label" as the
prefix_stras it'll go on every line. Maybe just use "" in place of "label" there?
It was in my initial v1. I thought it would be better to have. What about addresses? Would you like to see it or offset is enough, or drop them as well?
Moreover, I found another place where similar change could be done (dump_smb), so, would it be better to do in separate patch?
Quoted 8 lines
fs/cifs/cifs_debug.c line 43
Quoted 4 lines
fs/cifs/cifs_debug.c +0
-0}#ifdef CONFIG_CIFS_DEBUG Otherwise, this looks good.
Acked-by Jeff Layton <[email protected]>
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Quoted 48 lines · On Mon, 12 May 2014 19:15:09 +0300 Andy Shevchenko…
Quoted 38 lines · Jeff Layton
Quoted 35 lines · On Mon, 12 May 2014 18:15:20 +0300 Andy Shevchenko…
This patch converts custom dumper to use native
print_hex_dump()instead. The dump will have a given label and addresses per each line which differs it from the original code.Signed-off-by Andy Shevchenko <[email protected]>--- fs/cifs/cifs_debug.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)fs/cifs/cifs_debug.c +0
-16@@ -34,27 +34,10 @@void cifs_dump_mem (char *label,void *data,int length) { -int i, j; -int *intptr = data; -char *charptr = data; -char buf[10 ], line[80 ]; -printk (KERN_DEBUG "%s: dump of %d bytes of data at 0x%p\n" , label, length, data); -for (i =0 ; i < length; i +=16 ) { - line[0 ] =0 ; -for (j =0 ; (j <4 ) && (i + j *4 < length); j++) { -sprintf (buf," %08x" , intptr[i /4 + j]); -strcat (line, buf); - } - buf[0 ] =' ' ; - buf[2 ] =0 ; -for (j =0 ; (j <16 ) && (i + j < length); j++) { - buf[1 ] =isprint (charptr[i + j]) ? charptr[i + j] :'.' ; -strcat (line, buf);Quoted 8 lines
fs/cifs/cifs_debug.c line 40
Quoted 5 lines
fs/cifs/cifs_debug.c +2
-3- } -printk (KERN_DEBUG "%s\n" , line); - } +print_hex_dump (KERN_DEBUG , label,DUMP_PREFIX_ADDRESS ,16 ,4 , + data, length, true);I'm not sure we want "label" as the
prefix_stras it'll go on every line. Maybe just use "" in place of "label" there?
Quoted 1 line
It was in my initial v1. I thought it would be better to have.
Most of the labels are stuff like "Bad SMB:" and I'm not sure there's a lot of value in repeating that on every line. The existing code certainly doesn't and that sort of thing tends to clutter up logfiles.
Quoted 3 lines
What about addresses? Would you like to see it or offset is enough, or drop them as well?
The addresses aren't terribly helpful in general, and printing them might be considered information leakage. I'd suggest going with just the offsets.
Quoted 3 lines
Moreover, I found another place where similar change could be done (
dump_smb), so, would it be better to do in separate patch?
Sure, that certainly wouldn't hurt. Your call on whether to do two patches or one.
Quoted 10 lines
Quoted 8 lines
fs/cifs/cifs_debug.c line 43
Quoted 4 lines
fs/cifs/cifs_debug.c +0
-0}#ifdef CONFIG_CIFS_DEBUG Otherwise, this looks good.
Acked-by Jeff Layton <[email protected]>
Signature · 1 line
Jeff Layton <[email protected]>
A message that is not in the archive (sent off-list or not yet received): 20140512122356.1143f317-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org
Quoted 51 lines · Jeff Layton
Quoted 48 lines · On Mon, 12 May 2014 19:15:09 +0300 Andy Shevchenko…
Quoted 46 lines · Jeff Layton
Quoted 30 lines · On Mon, 12 May 2014 18:15:20 +0300 Andy Shevchenko…
This patch converts custom dumper to use native
print_hex_dump()instead. The dump will have a given label and addresses per each line which differs it from the original code.Signed-off-by Andy Shevchenko <[email protected]>--- fs/cifs/cifs_debug.c | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-)fs/cifs/cifs_debug.c +0
-11@@ -34,27 +34,10 @@void cifs_dump_mem (char *label,void *data,int length) { -int i, j; -int *intptr = data; -char *charptr = data; -char buf[10 ], line[80 ]; -printk (KERN_DEBUG "%s: dump of %d bytes of data at 0x%p\n" , label, length, data); -for (i =0 ; i < length; i +=16 ) { - line[0 ] =0 ; -for (j =0 ; (j <4 ) && (i + j *4 < length); j++) { -sprintf (buf," %08x" , intptr[i /4 + j]); -strcat (line, buf); - }fs/cifs/cifs_debug.c line 40
Quoted 10 lines
fs/cifs/cifs_debug.c +2
-8- buf[0 ] =' ' ; - buf[2 ] =0 ; -for (j =0 ; (j <16 ) && (i + j < length); j++) { - buf[1 ] =isprint (charptr[i + j]) ? charptr[i + j] :'.' ; -strcat (line, buf); - } -printk (KERN_DEBUG "%s\n" , line); - } +print_hex_dump (KERN_DEBUG , label,DUMP_PREFIX_ADDRESS ,16 ,4 , + data, length, true);I'm not sure we want "label" as the
prefix_stras it'll go on every line. Maybe just use "" in place of "label" there?
Quoted 5 lines
Quoted 1 line
It was in my initial v1. I thought it would be better to have.
Most of the labels are stuff like "Bad SMB:" and I'm not sure there's a lot of value in repeating that on every line. The existing code certainly doesn't and that sort of thing tends to clutter up logfiles.
Got it.
Quoted 8 lines
Quoted 4 lines
What about addresses? Would you like to see it or offset is enough, or drop them as well?
The addresses aren't terribly helpful in general, and printing them might be considered information leakage. I'd suggest going with just the offsets.
Yeah, but since printk at the top of function already prints virtual address... Maybe you could fix it in separate patch if you want, I leave as agreed — just offsets and no label.
Quoted 7 lines
Quoted 4 lines
Moreover, I found another place where similar change could be done (
dump_smb), so, would it be better to do in separate patch?Sure, that certainly wouldn't hurt. Your call on whether to do two patches or one.
I'm going to resend a whole patchseries as v3 with coverletter and changelog, since I missed CC you in previous patch, and AB's Reviewed-by. Also found rebase error which makes compilation fail. And printk->pr_* change as new patch as well, will be 3 all together.
Quoted 13 lines
Quoted 12 lines
Quoted 10 lines
fs/cifs/cifs_debug.c line 43
Quoted 6 lines
fs/cifs/cifs_debug.c +0
-0}#ifdef CONFIG_CIFS_DEBUG Otherwise, this looks good.
Acked-by Jeff Layton <[email protected]>
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy