[PATCH v2] cifs: call print_hex_dump instead of custom implementation

Post by 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(-)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index fa78b68..e7b87ce 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -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

Looks good, thanks for this. You can add my Reviewed-By.

--
/ Alexander Bokovoy

Jeff Layton

2014-05-12 15:42:47 UTC

On Mon, 12 May 2014 18:15:20 +0300

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?

Post by Andy Shevchenko
}
#ifdef CONFIG_CIFS_DEBUG

Otherwise, this looks good.

Acked-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+***@public.gmane.org>

Andy Shevchenko

2014-05-12 16:15:09 UTC

Post by Jeff Layton
On Mon, 12 May 2014 18:15:20 +0300

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?

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?

Post by Andy Shevchenko
}
#ifdef CONFIG_CIFS_DEBUG

Otherwise, this looks good.

--
Andy Shevchenko <***@intel.com>
Intel Finland Oy

Jeff Layton

2014-05-12 16:23:56 UTC

On Mon, 12 May 2014 19:15:09 +0300

Post by Jeff Layton
On Mon, 12 May 2014 18:15:20 +0300

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?

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.

Post by Andy Shevchenko
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.

Post by Andy Shevchenko
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.

Post by Andy Shevchenko
}
#ifdef CONFIG_CIFS_DEBUG

Otherwise, this looks good.

--
Jeff Layton <***@poochiereds.net>

Andy Shevchenko

2014-05-12 16:30:05 UTC

Post by Jeff Layton
On Mon, 12 May 2014 19:15:09 +0300
=20

Post by Jeff Layton
On Mon, 12 May 2014 18:15:20 +0300
=20

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.
=20

om>

---
fs/cifs/cifs_debug.c | 21 ++-------------------
1 file changed, 2 insertions(+), 19 deletions(-)
=20
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index fa78b68..e7b87ce 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -34,27 +34,10 @@
void
cifs_dump_mem(char *label, void *data, int length)
{
- int i, j;
- int *intptr =3D data;
- char *charptr =3D 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 =3D 0; i < length; i +=3D 16) {
- line[0] =3D 0;
- for (j =3D 0; (j < 4) && (i + j * 4 < length); j++) {
- sprintf(buf, " %08x", intptr[i / 4 + j]);
- strcat(line, buf);
- }
- buf[0] =3D ' ';
- buf[2] =3D 0;
- for (j =3D 0; (j < 16) && (i + j < length); j++) {
- buf[1] =3D 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);

=20
I'm not sure we want "label" as the prefix_str as it'll go on eve=

Post by Jeff Layton
line. Maybe just use "" in place of "label" there?

=20
It was in my initial v1. I thought it would be better to have.

=20
Most of the labels are stuff like "Bad SMB:" and I'm not sure there's=

Post by Jeff Layton
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=

=2E

Got it.

Post by Jeff Layton
=20

Post by Andy Shevchenko
What about addresses? Would you like to see it or offset is enough,=

Post by Andy Shevchenko
drop them as well?
=20

=20
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 leav=
e
as agreed =E2=80=94 just offsets and no label.

Post by Jeff Layton
=20

Post by Andy Shevchenko
Moreover, I found another place where similar change could be done
(dump_smb), so, would it be better to do in separate patch?
=20

=20
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.

Post by Jeff Layton
=20
=20