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: <87msdgzz40.fsf@bootlin.com>
Date: Wed, 19 Mar 2025 18:53:03 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.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 19/03/2025 at 18:37:26 +02, Andy Shevchenko <andriy.shevchenko@...ux.intel.com> 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