linux-kernel - Re: [PATCH v3 2/3] hexdump: Allow skipping identical lines

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] [day] [month] [year] [list]

Message-ID: <87tt7oyjuj.fsf@bootlin.com>
Date: Wed, 19 Mar 2025 19:08:04 +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 2/3] hexdump: Allow skipping identical lines

>>  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`.
>
> Instead of fixing this (see also comment in previous patch), just add the text
> like
>
> :c:func:`print_hex` is  especially useful since duplicated lines can be skipped
> automatically to reduce the overhead with the
> ``DUMP_SKIP_IDENTICAL_LINES`` flag.

Why are you bothered here? I am sorry but this part of the review is
absolutely pointless. I have no words to emphasize how high the
nitpicking level is. Please refrain yourself.

>> +:c:func:`print_hex`, especially since duplicated lines can be
>> +skipped automatically to reduce the overhead with the
>> +``DUMP_SKIP_IDENTICAL_LINES`` flag.
>
> Also, can  we also put a sub name spaces to the flags, like for HEX/ASCII
>
> DUMP_DATA_HEX
> DUMP_DATA_ASCII

They just refer to the way the data is dumped, so "data" is in the name,
that's true. The fact that they share the same namespace is fortunate
but not super relevant either. I am following the hints discussed in v2
regarding the naming, I am not too attached to these, but they felt
correct, so I use them.

> This SKIP  will start a new sub name space.

Yes. And?

I would have probably chosen a common name space from day 1 if I was
creating these now, but they are already two enums named
"DUMP_PREFIX". I guess we all agree it would be stupid to prefix all
enums "DUMP_PREFIX" anyway? And because you disgusted me from attempting
brave tree-wide changes, I will not rename them.

>
> ...
>
>>  #include <linux/errno.h>
>>  #include <linux/kernel.h>
>>  #include <linux/minmax.h>
>
>> +#include <linux/string.h>
>>  #include <linux/export.h>
>
> It's more natural to put it here, with given context it makes more order
> (speaking of alphabetical one).

If I learned something on these mailing lists, it is that what is
natural for me might not be for others. The alphabetical order is not
respected. You already pointed that in your first review, so I chose
another place which felt more relevant... to me.
e, k, m, s, u. This is the alphabetical order. export.h is not at the
correct place, I am very sorry.