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

[Andy Shevchenko]

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()

> 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

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

...

> +/*
> + * 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?

Also you can use bit mask and two bits for the value:

	DUMP_PREFIX_MASK = GENMASK(1, 0)

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

...

> +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?

-- 
With Best Regards,
Andy Shevchenko