Re: [PATCH v3 1/3] hexdump: Simplify print_hex_dump()

[Miquel Raynal]

On 19/03/2025 at 18:37:26 +02, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> On Wed, Mar 19, 2025 at 05:08:10PM +0100, Miquel Raynal wrote:
>> print_hex_dump() already has numerous parameters, and could be extended
>> with a new one. Adding new parameters is super painful due to the number
>> of users, and it makes the function calls even longer.
>> 
>> Create a print_hex() to replace print_hex_dump(), with 'prefix_type' and
>> 'ascii' being merged into a 'dump_flags' parameter. This way extending
>> the list of dump flags will be much easier.
>> 
>> For convenience, a print_hex_dump macro is created to fallback on the
>
> print_hex_dump()

It is a macro, not a function, so I don't feel bothered by the absence
of parenthesis. Anyway, that's really a nitpick, so if you want, I'll
add them.

>> print_hex() implementation. A tree-wide change to remove its use could
>> be done in the future.
>> 
>> No functional change intended.
>
> ...
>
>>  For printing small buffers (up to 64 bytes long) as a hex string with a
>>  certain separator. For larger buffers consider using
>> -:c:func:`print_hex_dump`.
>> +:c:func:`print_hex`.
>
> Why replacement? I would rather expect

Because it is a replacement. I initially wanted a tree-wide change but
it is too heavy and painful to carry. So I am replacing print_hex_dump()
by print_hex() as it was discussed in v2, but keeping print_hex_dump()
possible. In practice it is a handy fallback on print_hex(), nothing
else.

> :c:func:`print_hex_dump` or :c:func:`print_hex` depending on your
> needs.

There is no need print_hex_dump() fills and print_hex() does not. It
is actually the opposite. We no longer need print_hex_dump().

>
> ...
>
>> +/*
>> + * Dump flags for print_hex().
>> + * DUMP_PREFIX_{NONE,ADDRESS,OFFSET} are mutually exclusive.
>
> This is confusing, taking into account two definitions to 0.
>> + */
>>  enum {
>> +	DUMP_HEX_DATA = 0,
>> +	DUMP_ASCII = BIT(0),
>> +	DUMP_PREFIX_NONE = 0, /* Legacy definition for print_hex_dump() */
>> +	DUMP_PREFIX_ADDRESS = BIT(1),
>> +	DUMP_PREFIX_OFFSET = BIT(2),
>>  };
>
> Can we rather add a new enum and leave this untouched?

No, because the DUMP_PREFIX_ADDRESS/OFFSET are needed in
both. DUMP_PREFIX_NONE is no longer really needed, that's why I mark it
legacy with a comment, it's presence or absence no longer matters with
print_hex().

> Also you can use bit mask and two bits for the value:
>
> 	DUMP_PREFIX_MASK = GENMASK(1, 0)

Why? What is the use case?

> and no need to have the above comment about exclusiveness and no need to change
> the values.

Exclusiveness has always been there, just look at the code, I'm not
adding anything new. Refusing to change values for an enumeration is
totally pointless, it has no impact, no cost, no consequence. I don't
see your point.

>
> ...
>
>> +extern void print_hex(const char *level, const char *prefix_str,
>> +		      int rowsize, int groupsize,
>> +		      const void *buf, size_t len,
>> +		      unsigned int dump_flags);
>
>> +static inline void print_hex(const char *level, const char *prefix_str,
>> +			     int rowsize, int groupsize,
>> +			     const void *buf, size_t len,
>> +			     unsigned int dump_flags)
>
> Hmm... Wouldn't you want to have a enum as a last parameter?

And this has already been discussed in v2, we need to pass multiple
flags and decided to go for an unsigned int|long, I do not think the
compiler will like it otherwise.

Regards,
Miquèl