linux-kernel - Re: [PATCH v2 2/2] 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: <Z44kojleQta1bfoe@pathway.suse.cz>
Date: Mon, 20 Jan 2025 11:25:38 +0100
From: Petr Mladek <pmladek@...e.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: David Laight <david.laight.linux@...il.com>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.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 v2 2/2] hexdump: Allow skipping identical lines

On Mon 2025-01-20 10:29:27, Miquel Raynal wrote:
> Hello David & Petr,
> 
> On 17/01/2025 at 19:25:22 GMT, David Laight <david.laight.linux@...il.com> wrote:
> 
> > On Fri, 17 Jan 2025 17:27:26 +0100
> > Petr Mladek <pmladek@...e.com> wrote:
> >
> > ...
> >> IMHO, it is perfectly fine to add support for skipping identical lines
> >> only to print_hex_dump(). And I would go even further and replace
> >> 
> >> void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> >> 		    int rowsize, int groupsize,
> >> 		    const void *buf, size_t len, bool ascii)
> >> 
> >> with
> >> 
> >> void print_hex_dump(const char *level, const char *prefix_str,
> >> 		    enum hex_dump_type,
> >> 		    int rowsize, int groupsize,
> >> 		    const void *buf, size_t len)
> >> 
> >> and combine all the flags into the one enum:
> >> 
> >> enum hex_dump_type {
> >> 	DUMP_HEX_ONLY = 0,
> >> 	DUMP_HEX_AND_ASCII = BIT(1),
> >> 	DUMP_PREFIX_ADDRESS = BIT(2),
> >> 	DUMP_PREFIX_OFFSET = BIT(3),
> >> 	DUMP_SKIP_IDENTICAL_LINES = BIT(4),
> >> };
> 
> Would a single enum (in the prototype of the function) work? I like the
> idea but we need some kind of OR combination to be supported, typically:
>
> DUMP_HEX_AND_ASCII | DUMP_PREFIX_OFFSET | DUMP_SKIP_IDENTICAL_LINES
> 
> Maybe something like:
> 
> void print_hex(const char *level, const char *prefix_str,
> 	       int rowsize, int groupsize,
> 	       const void *buf, size_t len,
>                unsigned int dump_flags) // flags instead of enum?

Yes, the parameter would need to be an unsigned int or unsigned long
type so that we could use the logical OR operation.

We could also define the bits without the enum type because the enum
type won't be used anywhere.

I just thought that you wanted to have it "enum". AFAIK, workqueues
code uses enum because it allows to use the names of the bits in
crash/gdb. The enum will cause that the names of the values will
appear in the debug info...

> enum hex_dump_flags {
>         // I'm not sure what to do with the default value?

I would define

	DUMP_HEX_ONLY = 0,
or
	DUMP_HEX_DATA = 0,

It would be used only when the caller wants only the plain hex data.

> 	DUMP_ASCII = BIT(0), // renamed?

You are right that DUMP_ASCII might be enough. The names of the flags
would mean what the caller wants on top of the plain hex data.

> 	DUMP_PREFIX_ADDRESS = BIT(1),
> 	DUMP_PREFIX_OFFSET = BIT(2),
> 	DUMP_SKIP_IDENTICAL_LINES = BIT(3),
> };


> >> How does that sound, please?
> >
> > Rename it as (say) print_hex() and add wrappers for the existing callers?
> 
> That would avoid the treewide changes, so yes I can try that, definitely.

I am fine with it. But a treewide clean up (2 parameters -> one flags)
would be better from my POV.

Best Regards,
Petr