Re: [PATCH v3 0/7] Hexdump Enhancements

[Jani Nikula]

On Wed, 19 Jun 2019, Joe Perches <joe@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>> > > > > 
>> > > > > 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.

-- 
Jani Nikula, Intel Open Source Graphics Center