Re: [PATCH v3 0/7] Hexdump Enhancements

On Wed, 19 Jun 2019, Joe Perches [off-list ref] wrote:

On Thu, 2019-06-20 at 11:14 +1000, Alastair D'Silva wrote:

quoted

On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:

quoted

On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:

quoted

On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:

quoted

On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:

quoted

From: Alastair D'Silva <redacted>

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.

quoted

quoted

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.

quoted

quoted

quoted

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.

-- 
Jani Nikula, Intel Open Source Graphics Center