[1/4] lib/hexdump.c: Allow 64 bytes per line

Commit Message

Alastair D'Silva April 10, 2019, 3:17 a.m. UTC

From: Alastair D'Silva <[e-mail]>

With modern high resolution screens, we can display more data, which makes
life a bit easier when debugging.

Allow 64 bytes to be dumped per line.

Signed-off-by: Alastair D'Silva <[e-mail]>
---
 lib/hexdump.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Petr Mladek April 12, 2019, 1:48 p.m. UTC | #1

On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> From: Alastair D'Silva <[e-mail]>
> 
> With modern high resolution screens, we can display more data, which makes
> life a bit easier when debugging.

I have quite some doubts about this feature.

We are talking about more than 256 characters per-line. I wonder
if such a long line is really easier to read for a human.

I am not expert but there is a reason why the standard is 80
characters per-line. I guess that anything above 100 characters
is questionable. https://en.wikipedia.org/wiki/Line_length
somehow confirms that.

Second, if we take 8 pixels per-character. Then we need
2048 to show the 256 characters. It is more than HD.
IMHO, there is still huge number of people that even do
not have HD display, especially on a notebook.


I am sorry but I am strongly against having this hardcoded
anywhere. And I doubt that there will be enough users
to complicate the code and make it configurable.

Best Regards,
Petr

Alastair D'Silva April 12, 2019, 11:22 p.m. UTC | #2

> -----Original Message-----
> From: Petr Mladek <[e-mail]>
> Sent: Friday, 12 April 2019 11:48 PM
> To: Alastair D'Silva <[e-mail]>
> Cc: [e-mail]; Jani Nikula <[e-mail]>;
Joonas
> Lahtinen <[e-mail]>; Rodrigo Vivi
> <[e-mail]>; David Airlie <[e-mail]>; Daniel Vetter
> <[e-mail]>; Karsten Keil <[e-mail]>; Jassi Brar
> <[e-mail]>; Tom Lendacky <[e-mail]>;
> David S. Miller <[e-mail]>; Jose Abreu
> <[e-mail]>; Kalle Valo <[e-mail]>;
> Stanislaw Gruszka <[e-mail]>; Benson Leung
> <[e-mail]>; Enric Balletbo i Serra
> <[e-mail]>; James E.J. Bottomley
> <[e-mail]>; Martin K. Petersen <[e-mail]>;
> Greg Kroah-Hartman <[e-mail]>; Alexander Viro
> <[e-mail]>; Sergey Senozhatsky
> <[e-mail]>; Steven Rostedt <[e-mail]>;
> Andrew Morton <[e-mail]>; intel-
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line
> 
> On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> > From: Alastair D'Silva <[e-mail]>
> >
> > With modern high resolution screens, we can display more data, which
> > makes life a bit easier when debugging.
> 
> I have quite some doubts about this feature.
> 
> We are talking about more than 256 characters per-line. I wonder if such a
> long line is really easier to read for a human.

It's basically 2 separate panes of information side by side, the hexdump and
the ASCII version.

I'm using this myself when dealing with the pmem labels, and it works quite
nicely.

> 
> I am not expert but there is a reason why the standard is 80 characters
per-
> line. I guess that anything above 100 characters is questionable.
> https://en.wikipedia.org/wiki/Line_length
> somehow confirms that.
> 
> Second, if we take 8 pixels per-character. Then we need
> 2048 to show the 256 characters. It is more than HD.
> IMHO, there is still huge number of people that even do not have HD
display,
> especially on a notebook.

The intent is to make debugging easier when dealing with large chunks of
binary data. I don't expect end users to see this output.

Petr Mladek April 15, 2019, 9:02 a.m. UTC | #3

On Sat 2019-04-13 09:22:05, Alastair D'Silva wrote:
> > -----Original Message-----
> > From: Petr Mladek <[e-mail]>
> > Sent: Friday, 12 April 2019 11:48 PM
> > To: Alastair D'Silva <[e-mail]>
> > Cc: [e-mail]; Jani Nikula <[e-mail]>;
> Joonas
> > Lahtinen <[e-mail]>; Rodrigo Vivi
> > <[e-mail]>; David Airlie <[e-mail]>; Daniel Vetter
> > <[e-mail]>; Karsten Keil <[e-mail]>; Jassi Brar
> > <[e-mail]>; Tom Lendacky <[e-mail]>;
> > David S. Miller <[e-mail]>; Jose Abreu
> > <[e-mail]>; Kalle Valo <[e-mail]>;
> > Stanislaw Gruszka <[e-mail]>; Benson Leung
> > <[e-mail]>; Enric Balletbo i Serra
> > <[e-mail]>; James E.J. Bottomley
> > <[e-mail]>; Martin K. Petersen <[e-mail]>;
> > Greg Kroah-Hartman <[e-mail]>; Alexander Viro
> > <[e-mail]>; Sergey Senozhatsky
> > <[e-mail]>; Steven Rostedt <[e-mail]>;
> > Andrew Morton <[e-mail]>; intel-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH 1/4] lib/hexdump.c: Allow 64 bytes per line
> > 
> > On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <[e-mail]>
> > >
> > > With modern high resolution screens, we can display more data, which
> > > makes life a bit easier when debugging.
> > 
> > I have quite some doubts about this feature.
> > 
> > We are talking about more than 256 characters per-line. I wonder if such a
> > long line is really easier to read for a human.
> 
> It's basically 2 separate panes of information side by side, the hexdump and
> the ASCII version.
> 
> I'm using this myself when dealing with the pmem labels, and it works quite
> nicely.

I am sure that it works for you. But I do not believe that it
would be useful in general.


> > I am not expert but there is a reason why the standard is 80 characters
> per-
> > line. I guess that anything above 100 characters is questionable.
> > https://en.wikipedia.org/wiki/Line_length
> > somehow confirms that.
> > 
> > Second, if we take 8 pixels per-character. Then we need
> > 2048 to show the 256 characters. It is more than HD.
> > IMHO, there is still huge number of people that even do not have HD
> display,
> > especially on a notebook.
> 
> The intent is to make debugging easier when dealing with large chunks of
> binary data. I don't expect end users to see this output.

How is it supposed to be used then? Only by your temporary patches?

Best Regards,
Petr

Alastair D'Silva April 15, 2019, 10:29 a.m. UTC | #4

<snip>
> > > On Wed 2019-04-10 13:17:17, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <[e-mail]>
> > > >
> > > > With modern high resolution screens, we can display more data,
> > > > which makes life a bit easier when debugging.
> > >
> > > I have quite some doubts about this feature.
> > >
> > > We are talking about more than 256 characters per-line. I wonder if
> > > such a long line is really easier to read for a human.
> >
> > It's basically 2 separate panes of information side by side, the
> > hexdump and the ASCII version.
> >
> > I'm using this myself when dealing with the pmem labels, and it works
> > quite nicely.
> 
> I am sure that it works for you. But I do not believe that it would be
useful in
> general.

I do, and I believe the choice of the output length should be in the hands
of the caller.

On further thought, it would make more sense to remove the hardcoded list of
sizes and just enforce a power of 2. The function shouldn't dictate what the
caller can and can't do beyond the technical limits of it's implementation.

Other print/debug functions don't restrict the output size, and I can't see
a good justification why hexdump should either.

> > > I am not expert but there is a reason why the standard is 80
> > > characters
> > per-
> > > line. I guess that anything above 100 characters is questionable.
> > > https://en.wikipedia.org/wiki/Line_length
> > > somehow confirms that.
> > >
> > > Second, if we take 8 pixels per-character. Then we need
> > > 2048 to show the 256 characters. It is more than HD.
> > > IMHO, there is still huge number of people that even do not have HD
> > display,
> > > especially on a notebook.
> >
> > The intent is to make debugging easier when dealing with large chunks
> > of binary data. I don't expect end users to see this output.
> 
> How is it supposed to be used then? Only by your temporary patches?

Let me rephrase that, I don't expect end users to *use* this data.

Current usage of the hexdump functions are predominantly centred around
logging and debugging, and clearly targeted at someone intimately familiar
with the relevant subsystem. I expect future use would be similar.

Debugging may be as part of active development, or from a log supplied from
an end user. In either case, it should be up to the author (as a
representative for the consumers of the data) to decide how it should be
formatted.

David Laight April 15, 2019, 10:56 a.m. UTC | #5

From: Alastair D'Silva
> Sent: 15 April 2019 11:29
...
> I do, and I believe the choice of the output length should be in the hands
> of the caller.
> 
> On further thought, it would make more sense to remove the hardcoded list of
> sizes and just enforce a power of 2. The function shouldn't dictate what the
> caller can and can't do beyond the technical limits of it's implementation.

Why powers of two?
You may want the length to match sizeof (struct foo).
You might even want the address increment to be larger
that the number of lines dumped.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Alastair D'Silva April 15, 2019, 10:59 a.m. UTC | #6

> From: Alastair D'Silva
> > Sent: 15 April 2019 11:29
> ...
> > I do, and I believe the choice of the output length should be in the
> > hands of the caller.
> >
> > On further thought, it would make more sense to remove the hardcoded
> > list of sizes and just enforce a power of 2. The function shouldn't
> > dictate what the caller can and can't do beyond the technical limits of it's
> implementation.
> 
> Why powers of two?
> You may want the length to match sizeof (struct foo).
> You might even want the address increment to be larger that the number of
> lines dumped.

Good point, the base requirement is that it should be a multiple of groupsize.

Patch

diff --git a/lib/hexdump.c b/lib/hexdump.c
index 81b70ed37209..b8a164814744 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -80,14 +80,14 @@  EXPORT_SYMBOL(bin2hex);
  * hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
- * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @rowsize: number of bytes to print per line; must be 16, 32 or 64
  * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
  * @linebuf: where to put the converted data
  * @linebuflen: total size of @linebuf, including space for terminating NUL
  * @ascii: include ASCII after the hex output
  *
  * hex_dump_to_buffer() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
+ * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
  *
  * Given a buffer of u8 data, hex_dump_to_buffer() converts the input data
  * to a hex + ASCII dump at the supplied memory location.
@@ -116,7 +116,7 @@  int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
 	int ascii_column;
 	int ret;
 
-	if (rowsize != 16 && rowsize != 32)
+	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
 		rowsize = 16;
 
 	if (len > rowsize)		/* limit to one line at a time */
@@ -216,7 +216,7 @@  EXPORT_SYMBOL(hex_dump_to_buffer);
  *  caller supplies trailing spaces for alignment if desired
  * @prefix_type: controls whether prefix of an offset, address, or none
  *  is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
- * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @rowsize: number of bytes to print per line; must be 16, 32 or 64
  * @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
  * @buf: data blob to dump
  * @len: number of bytes in the @buf
@@ -227,7 +227,7 @@  EXPORT_SYMBOL(hex_dump_to_buffer);
  * leading prefix.
  *
  * print_hex_dump() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
+ * 16, 32 or 64 bytes of input data converted to hex + ASCII output.
  * print_hex_dump() iterates over the entire input @buf, breaking it into
  * "line size" chunks to format and print.
  *
@@ -246,9 +246,9 @@  void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
 {
 	const u8 *ptr = buf;
 	int i, linelen, remaining = len;
-	unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+	unsigned char linebuf[64 * 3 + 2 + 64 + 1];
 
-	if (rowsize != 16 && rowsize != 32)
+	if (rowsize != 16 && rowsize != 32 && rowsize != 64)
 		rowsize = 16;
 
 	for (i = 0; i < len; i += rowsize) {