[v3,0/7] Hexdump Enhancements
mbox series

Message ID 20190617020430.8708-1-alastair@au1.ibm.com
Headers show
Series
  • Hexdump Enhancements
Related show

Message

Alastair D'Silva June 17, 2019, 2:04 a.m. UTC
From: Alastair D'Silva <alastair@d-silva.org>

Apologies for the large CC list, it's a heads up for those responsible
for subsystems where a prototype change in generic code causes a change
in those subsystems.

This series enhances hexdump.

These improve the readability of the dumped data in certain situations
(eg. wide terminals are available, many lines of empty bytes exist, etc).

The default behaviour of hexdump is unchanged, however, the prototype
for hex_dump_to_buffer() has changed, and print_hex_dump() has been
renamed to print_hex_dump_ext(), with a wrapper replacing it for
compatibility with existing code, which would have been too invasive to
change.

Hexdump selftests have be run & confirmed passed.

Changelog:
V3:
 - Fix inline documention
 - use BIT macros
 - use u32 rather than u64 for flags
V2:
 - Fix failing selftests
 - Fix precedence bug in 'Replace ascii bool in hex_dump_to_buffer...'
 - Remove hardcoded new lengths & instead relax the checks in
   hex_dump_to_buffer, allocating the buffer from the heap instead of the
   stack.
 - Replace the skipping of lines of 0x00/0xff with skipping lines of
   repeated characters, announcing what has been skipped.
 - Add spaces as an optional N-group separator
 - Allow byte ordering to be maintained when HEXDUMP_RETAIN_BYTE_ORDERING
   is set.
 - Updated selftests to cover 'Relax rowsize checks' &
   'Optionally retain byte ordering'

Alastair D'Silva (7):
  lib/hexdump.c: Fix selftests
  lib/hexdump.c: Relax rowsize checks in hex_dump_to_buffer
  lib/hexdump.c: Optionally suppress lines of repeated bytes
  lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
  lib/hexdump.c: Allow multiple groups to be separated by lines '|'
  lib/hexdump.c: Allow multiple groups to be separated by spaces
  lib/hexdump.c: Optionally retain byte ordering

 drivers/gpu/drm/i915/intel_engine_cs.c        |   2 +-
 drivers/isdn/hardware/mISDN/mISDNisar.c       |   6 +-
 drivers/mailbox/mailbox-test.c                |   2 +-
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c      |   2 +-
 .../net/ethernet/synopsys/dwc-xlgmac-common.c |   2 +-
 drivers/net/wireless/ath/ath10k/debug.c       |   3 +-
 .../net/wireless/intel/iwlegacy/3945-mac.c    |   2 +-
 drivers/platform/chrome/wilco_ec/debugfs.c    |   2 +-
 drivers/scsi/scsi_logging.c                   |   8 +-
 drivers/staging/fbtft/fbtft-core.c            |   2 +-
 fs/seq_file.c                                 |   3 +-
 include/linux/printk.h                        |  34 ++-
 lib/hexdump.c                                 | 260 +++++++++++++++---
 lib/test_hexdump.c                            | 146 +++++++---
 14 files changed, 372 insertions(+), 102 deletions(-)

Comments

Joe Perches June 19, 2019, 4:31 p.m. UTC | #1
On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
> 
> Apologies for the large CC list, it's a heads up for those responsible
> for subsystems where a prototype change in generic code causes a change
> in those subsystems.
> 
> This series enhances hexdump.

Still not a fan of these patches.

> These improve the readability of the dumped data in certain situations
> (eg. wide terminals are available, many lines of empty bytes exist, etc).

Changing hexdump's last argument from bool to int is odd.

Perhaps a new function should be added instead of changing
the existing hexdump.

> The default behaviour of hexdump is unchanged, however, the prototype
> for hex_dump_to_buffer() has changed, and print_hex_dump() has been
> renamed to print_hex_dump_ext(), with a wrapper replacing it for
> compatibility with existing code, which would have been too invasive to
> change.
> 
> Hexdump selftests have be run & confirmed passed.
Alastair D'Silva June 19, 2019, 11:15 p.m. UTC | #2
On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <alastair@d-silva.org>
> > 
> > Apologies for the large CC list, it's a heads up for those
> > responsible
> > for subsystems where a prototype change in generic code causes a
> > change
> > in those subsystems.
> > 
> > This series enhances hexdump.
> 
> Still not a fan of these patches.

I'm afraid there's not too much action I can take on that, I'm happy to
address specific issues though.

> 
> > These improve the readability of the dumped data in certain
> > situations
> > (eg. wide terminals are available, many lines of empty bytes exist,
> > etc).
> 
> Changing hexdump's last argument from bool to int is odd.
> 

Think of it as replacing a single boolean with many booleans.

> Perhaps a new function should be added instead of changing
> the existing hexdump.
> 

There's only a handful of consumers, I don't think there is a value-add 
in creating more wrappers vs updating the existing callers.
Joe Perches June 20, 2019, 12:35 a.m. UTC | #3
On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
> On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > > From: Alastair D'Silva <alastair@d-silva.org>
> > > 
> > > Apologies for the large CC list, it's a heads up for those
> > > responsible
> > > for subsystems where a prototype change in generic code causes a
> > > change
> > > in those subsystems.
> > > 
> > > This series enhances hexdump.
> > 
> > Still not a fan of these patches.
> 
> I'm afraid there's not too much action I can take on that, I'm happy to
> address specific issues though.
> 
> > > These improve the readability of the dumped data in certain
> > > situations
> > > (eg. wide terminals are available, many lines of empty bytes exist,
> > > etc).

I think it's generally overkill for the desired uses.

> > Changing hexdump's last argument from bool to int is odd.
> > 
> 
> Think of it as replacing a single boolean with many booleans.

I understand it.  It's odd.

I would rather not have a mixture of true, false, and apparently
random collections of bitfields like 0xd or 0b1011 or their
equivalent or'd defines.


> There's only a handful of consumers, I don't think there is a value-add 
> in creating more wrappers vs updating the existing callers.

Perhaps more reason not to modify the existing api.
Alastair D'Silva June 20, 2019, 1:14 a.m. UTC | #4
On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
> On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
> > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > 
> > > > Apologies for the large CC list, it's a heads up for those
> > > > responsible
> > > > for subsystems where a prototype change in generic code causes
> > > > a
> > > > change
> > > > in those subsystems.
> > > > 
> > > > This series enhances hexdump.
> > > 
> > > Still not a fan of these patches.
> > 
> > I'm afraid there's not too much action I can take on that, I'm
> > happy to
> > address specific issues though.
> > 
> > > > These improve the readability of the dumped data in certain
> > > > situations
> > > > (eg. wide terminals are available, many lines of empty bytes
> > > > exist,
> > > > etc).
> 
> I think it's generally overkill for the desired uses.

I understand where you're coming from, however, these patches make it a
lot easier to work with large chucks of binary data. I think it makes
more sense to have these patches upstream, even though committed code
may not necessarily have all the features enabled, as it means that
devs won't have to apply out-of-tree patches during development to make
larger dumps manageable.

> 
> > > Changing hexdump's last argument from bool to int is odd.
> > > 
> > 
> > Think of it as replacing a single boolean with many booleans.
> 
> I understand it.  It's odd.
> 
> I would rather not have a mixture of true, false, and apparently
> random collections of bitfields like 0xd or 0b1011 or their
> equivalent or'd defines.
> 

Where's the mixture? What would you propose instead?
Joe Perches June 20, 2019, 2 a.m. UTC | #5
On Thu, 2019-06-20 at 11:14 +1000, Alastair D'Silva wrote:
> On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
> > On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
> > > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> > > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > > > > From: Alastair D'Silva <alastair@d-silva.org>
> > > > > 
> > > > > Apologies for the large CC list, it's a heads up for those
> > > > > responsible
> > > > > for subsystems where a prototype change in generic code causes
> > > > > a
> > > > > change
> > > > > in those subsystems.
> > > > > 
> > > > > This series enhances hexdump.
> > > > 
> > > > Still not a fan of these patches.
> > > 
> > > I'm afraid there's not too much action I can take on that, I'm
> > > happy to
> > > address specific issues though.
> > > 
> > > > > These improve the readability of the dumped data in certain
> > > > > situations
> > > > > (eg. wide terminals are available, many lines of empty bytes
> > > > > exist,
> > > > > etc).
> > 
> > I think it's generally overkill for the desired uses.
> 
> I understand where you're coming from, however, these patches make it a
> lot easier to work with large chucks of binary data. I think it makes
> more sense to have these patches upstream, even though committed code
> may not necessarily have all the features enabled, as it means that
> devs won't have to apply out-of-tree patches during development to make
> larger dumps manageable.
> 
> > > > Changing hexdump's last argument from bool to int is odd.
> > > > 
> > > 
> > > Think of it as replacing a single boolean with many booleans.
> > 
> > I understand it.  It's odd.
> > 
> > I would rather not have a mixture of true, false, and apparently
> > random collections of bitfields like 0xd or 0b1011 or their
> > equivalent or'd defines.
> > 
> 
> Where's the mixture? What would you propose instead?

create a hex_dump_to_buffer_ext with a new argument
and a new static inline for the old hex_dump_to_buffer
without modifying the argument list that calls
hex_dump_to_buffer with whatever added argument content
you need.

Something like:

static inline
int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
		       int groupsize, char *linebuf, size_t linebuflen,
		       bool ascii)
{
	return hex_dump_to_buffer_ext(buf, len, rowsize, groupsize,
				      linebuf, linebuflen, ascii, 0);
}

and remove EXPORT_SYMBOL(hex_dump_to_buffer)
Jani Nikula June 20, 2019, 10:50 a.m. UTC | #6
On Wed, 19 Jun 2019, Joe Perches <joe@perches.com> wrote:
> On Thu, 2019-06-20 at 11:14 +1000, Alastair D'Silva wrote:
>> On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
>> > On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
>> > > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
>> > > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
>> > > > > From: Alastair D'Silva <alastair@d-silva.org>
>> > > > > 
>> > > > > Apologies for the large CC list, it's a heads up for those
>> > > > > responsible
>> > > > > for subsystems where a prototype change in generic code causes
>> > > > > a
>> > > > > change
>> > > > > in those subsystems.
>> > > > > 
>> > > > > This series enhances hexdump.
>> > > > 
>> > > > Still not a fan of these patches.
>> > > 
>> > > I'm afraid there's not too much action I can take on that, I'm
>> > > happy to
>> > > address specific issues though.
>> > > 
>> > > > > These improve the readability of the dumped data in certain
>> > > > > situations
>> > > > > (eg. wide terminals are available, many lines of empty bytes
>> > > > > exist,
>> > > > > etc).
>> > 
>> > I think it's generally overkill for the desired uses.
>> 
>> I understand where you're coming from, however, these patches make it a
>> lot easier to work with large chucks of binary data. I think it makes
>> more sense to have these patches upstream, even though committed code
>> may not necessarily have all the features enabled, as it means that
>> devs won't have to apply out-of-tree patches during development to make
>> larger dumps manageable.
>> 
>> > > > Changing hexdump's last argument from bool to int is odd.
>> > > > 
>> > > 
>> > > Think of it as replacing a single boolean with many booleans.
>> > 
>> > I understand it.  It's odd.
>> > 
>> > I would rather not have a mixture of true, false, and apparently
>> > random collections of bitfields like 0xd or 0b1011 or their
>> > equivalent or'd defines.
>> > 
>> 
>> Where's the mixture? What would you propose instead?
>
> create a hex_dump_to_buffer_ext with a new argument
> and a new static inline for the old hex_dump_to_buffer
> without modifying the argument list that calls
> hex_dump_to_buffer with whatever added argument content
> you need.
>
> Something like:
>
> static inline
> int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> 		       int groupsize, char *linebuf, size_t linebuflen,
> 		       bool ascii)
> {
> 	return hex_dump_to_buffer_ext(buf, len, rowsize, groupsize,
> 				      linebuf, linebuflen, ascii, 0);
> }
>
> and remove EXPORT_SYMBOL(hex_dump_to_buffer)

If you decide to do something like this, I'd actually suggest you drop
the bool ascii parameter from hex_dump_to_buffer() altogether, and
replace the callers that do require ascii with
hex_dump_to_buffer_ext(..., HEXDUMP_ASCII). Even if that also requires
touching all callers.

But no strong opinions, really.

BR,
Jani.