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() implementation. A tree-wide change to remove its use could
be done in the future.
No functional change intended.
Suggested-by: Petr Mladek <pmladek@suse.com>
Suggested-by: David Laight <david.laight.linux@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
Documentation/core-api/printk-formats.rst | 2 +-
include/linux/printk.h | 42 +++++++++++++++++++++++-------
lib/hexdump.c | 43 ++++++++++++++-----------------
3 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index ecccc0473da9c10f45f2464566f690472c61401e..f80b5e262e9933992d1291f1d78fba97589d5631 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -310,7 +310,7 @@ Raw buffer as a hex string
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`.
MAC/FDDI addresses
------------------
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 4217a9f412b265f1dc027be88eff7f5a0e0e4aab..7dca2270c82c0ed788cd706274f1c1b14ed9a7fe 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -747,22 +747,31 @@ do { \
extern const struct file_operations kmsg_fops;
+/*
+ * Dump flags for print_hex().
+ * DUMP_PREFIX_{NONE,ADDRESS,OFFSET} are mutually exclusive.
+ */
enum {
- DUMP_PREFIX_NONE,
- DUMP_PREFIX_ADDRESS,
- DUMP_PREFIX_OFFSET
+ 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),
};
+
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
bool ascii);
#ifdef CONFIG_PRINTK
-extern 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);
+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);
#else
-static inline 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)
+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)
{
}
static inline void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
@@ -791,6 +800,21 @@ static inline void print_hex_dump_debug(const char *prefix_str, int prefix_type,
}
#endif
+/*
+ * print_hex_dump - legacy version of print_hex() with a longer parameter list
+ *
+ * Refer to print_hex() for the parameters definition which are identical except:
+ * - prefix_type: controls whether prefix of an offset, address, or none
+ * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE).
+ * This parameter has been removed in favor of a common 'flags' parameter.
+ * - ascii: include ASCII after the hex output.
+ * This parameter has been removed in favor of a common 'flags' parameter.
+ */
+
+#define print_hex_dump(level, prefix_str, prefix_type, rowsize, groupsize, buf, len, ascii) \
+ print_hex(level, prefix_str, rowsize, groupsize, buf, len, \
+ (prefix_type) | ((ascii) ? DUMP_ASCII : DUMP_HEX_DATA))
+
/**
* print_hex_dump_bytes - shorthand form of print_hex_dump() with default params
* @prefix_str: string to prefix each line with;
diff --git a/lib/hexdump.c b/lib/hexdump.c
index c3db7c3a764365b01e78f8ed0b7f782f33a5ce34..74fdcb4566d27f257a0e1288c261d81d231b06bf 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -228,39 +228,39 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
#ifdef CONFIG_PRINTK
/**
- * print_hex_dump - print a text hex dump to syslog for a binary blob of data
+ * print_hex - print a text hex dump to syslog for a binary blob of data
* @level: kernel log level (e.g. KERN_DEBUG)
* @prefix_str: string to prefix each line with;
* caller supplies trailing spaces for alignment if desired
- * @prefix_type: controls whether prefix of an offset, address, or none
- * is printed (%DUMP_PREFIX_OFFSET, %DUMP_PREFIX_ADDRESS, %DUMP_PREFIX_NONE)
* @rowsize: number of bytes to print per line; must be 16 or 32
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @buf: data blob to dump
* @len: number of bytes in the @buf
- * @ascii: include ASCII after the hex output
+ * @dump_flags: controls the output format, typically:
+ * - %DUMP_PREFIX_OFFSET shows the offset in front of each line
+ * - %DUMP_PREFIX_ADDRESS shows the address in front of each line
+ * - %DUMP_ASCII prints the ascii equivalent after the hex output
*
- * Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
+ * Given a buffer of u8 data, print_hex() prints a hex + ASCII dump
* to the kernel log at the specified kernel log level, with an optional
* leading prefix.
*
- * print_hex_dump() works on one "line" of output at a time, i.e.,
+ * print_hex() works on one "line" of output at a time, i.e.,
* 16 or 32 bytes of input data converted to hex + ASCII output.
- * print_hex_dump() iterates over the entire input @buf, breaking it into
+ * print_hex() iterates over the entire input @buf, breaking it into
* "line size" chunks to format and print.
*
* E.g.:
- * print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
- * 16, 1, frame->data, frame->len, true);
+ * print_hex(KERN_DEBUG, "raw data: ", 16, 1, frame->data, frame->len,
+ * DUMP_PREFIX_ADDRESS | DUMP_ASCII);
*
* Example output using %DUMP_PREFIX_OFFSET and 1-byte mode:
* 0009ab42: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f @ABCDEFGHIJKLMNO
* Example output using %DUMP_PREFIX_ADDRESS and 4-byte mode:
* ffffffff88089af0: 73727170 77767574 7b7a7978 7f7e7d7c pqrstuvwxyz{|}~.
*/
-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)
+void print_hex(const char *level, const char *prefix_str, int rowsize, int groupsize,
+ const void *buf, size_t len, unsigned int dump_flags)
{
const u8 *ptr = buf;
int i, linelen, remaining = len;
@@ -274,22 +274,17 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
remaining -= rowsize;
hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
- linebuf, sizeof(linebuf), ascii);
+ linebuf, sizeof(linebuf),
+ dump_flags & DUMP_ASCII);
- switch (prefix_type) {
- case DUMP_PREFIX_ADDRESS:
- printk("%s%s%p: %s\n",
- level, prefix_str, ptr + i, linebuf);
- break;
- case DUMP_PREFIX_OFFSET:
+ if (dump_flags & DUMP_PREFIX_ADDRESS)
+ printk("%s%s%p: %s\n", level, prefix_str, ptr + i, linebuf);
+ else if (dump_flags & DUMP_PREFIX_OFFSET)
printk("%s%s%.8x: %s\n", level, prefix_str, i, linebuf);
- break;
- default:
+ else
printk("%s%s%s\n", level, prefix_str, linebuf);
- break;
- }
}
}
-EXPORT_SYMBOL(print_hex_dump);
+EXPORT_SYMBOL(print_hex);
#endif /* defined(CONFIG_PRINTK) */
--
2.48.1On 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
On 19/03/2025 at 18:37:26 +02, Andy Shevchenko <andriy.shevchenko@linux.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
© 2016 - 2025 Red Hat, Inc.