linux-kernel - Re: [PATCH v3 1/3] hexdump: Simplify print_hex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z9ryxqRd0u4cZJoL@smile.fi.intel.com>
Date: Wed, 19 Mar 2025 18:37:26 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: Petr Mladek <pmladek@...e.com>,
	David Laight <david.laight.linux@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Rasmus Villemoes <linux@...musvillemoes.dk>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Jonathan Corbet <corbet@....net>,
	John Ogness <john.ogness@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] hexdump: Simplify print_hex_dump()

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