From: Alastair D'Silva <[email protected]>
Apologies for the large CC list, it's a heads up for those responsible
for subsystems where a prototype change in generic code causes a change
in those subsystems.
This series enhances hexdump.
These improve the readability of the dumped data in certain situations
(eg. wide terminals are available, many lines of empty bytes exist, etc).
The default behaviour of hexdump is unchanged, however, the prototype
for hex_dump_to_buffer() has changed, and print_hex_dump() has been
renamed to print_hex_dump_ext(), with a wrapper replacing it for
compatibility with existing code, which would have been too invasive to
change.
Hexdump selftests have be run & confirmed passed.
Changelog:
V3:
- Fix inline documention
- use BIT macros
- use u32 rather than u64 for flags
V2:
- Fix failing selftests
- Fix precedence bug in 'Replace ascii bool in hex_dump_to_buffer...'
- Remove hardcoded new lengths & instead relax the checks in
hex_dump_to_buffer, allocating the buffer from the heap instead of the
stack.
- Replace the skipping of lines of 0x00/0xff with skipping lines of
repeated characters, announcing what has been skipped.
- Add spaces as an optional N-group separator
- Allow byte ordering to be maintained when HEXDUMP_RETAIN_BYTE_ORDERING
is set.
- Updated selftests to cover 'Relax rowsize checks' &
'Optionally retain byte ordering'
Alastair D'Silva (7):
lib/hexdump.c: Fix selftests
lib/hexdump.c: Relax rowsize checks in hex_dump_to_buffer
lib/hexdump.c: Optionally suppress lines of repeated bytes
lib/hexdump.c: Replace ascii bool in hex_dump_to_buffer with flags
lib/hexdump.c: Allow multiple groups to be separated by lines '|'
lib/hexdump.c: Allow multiple groups to be separated by spaces
lib/hexdump.c: Optionally retain byte ordering
drivers/gpu/drm/i915/intel_engine_cs.c | 2 +-
drivers/isdn/hardware/mISDN/mISDNisar.c | 6 +-
drivers/mailbox/mailbox-test.c | 2 +-
drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 2 +-
.../net/ethernet/synopsys/dwc-xlgmac-common.c | 2 +-
drivers/net/wireless/ath/ath10k/debug.c | 3 +-
.../net/wireless/intel/iwlegacy/3945-mac.c | 2 +-
drivers/platform/chrome/wilco_ec/debugfs.c | 2 +-
drivers/scsi/scsi_logging.c | 8 +-
drivers/staging/fbtft/fbtft-core.c | 2 +-
fs/seq_file.c | 3 +-
include/linux/printk.h | 34 ++-
lib/hexdump.c | 260 +++++++++++++++---
lib/test_hexdump.c | 146 +++++++---
14 files changed, 372 insertions(+), 102 deletions(-)
--
2.21.0
From: Alastair D'Silva <[email protected]>
This patch removes the hardcoded row limits and allows for
other lengths. These lengths must still be a multiple of
groupsize.
This allows structs that are not 16/32 bytes to display on
a single line.
This patch also expands the self-tests to test row sizes
up to 64 bytes (though they can now be arbitrarily long).
Signed-off-by: Alastair D'Silva <[email protected]>
---
lib/hexdump.c | 48 ++++++++++++++++++++++++++++--------------
lib/test_hexdump.c | 52 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 75 insertions(+), 25 deletions(-)
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 81b70ed37209..3943507bc0e9 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -12,6 +12,7 @@
#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/export.h>
+#include <linux/slab.h>
#include <asm/unaligned.h>
const char hex_asc[] = "0123456789abcdef";
@@ -80,14 +81,15 @@ EXPORT_SYMBOL(bin2hex);
* hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
* @buf: data blob to dump
* @len: number of bytes in the @buf
- * @rowsize: number of bytes to print per line; must be 16 or 32
+ * @rowsize: number of bytes to print per line; must be a multiple of groupsize
* @groupsize: number of bytes to print at a time (1, 2, 4, 8; default = 1)
* @linebuf: where to put the converted data
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @ascii: include ASCII after the hex output
*
- * hex_dump_to_buffer() works on one "line" of output at a time, i.e.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
+ * hex_dump_to_buffer() works on one "line" of output at a time, converting
+ * <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
+ * until <rowsize> bytes have been emitted.
*
* Given a buffer of u8 data, hex_dump_to_buffer() converts the input data
* to a hex + ASCII dump at the supplied memory location.
@@ -116,16 +118,17 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
int ascii_column;
int ret;
- if (rowsize != 16 && rowsize != 32)
- rowsize = 16;
-
- if (len > rowsize) /* limit to one line at a time */
- len = rowsize;
if (!is_power_of_2(groupsize) || groupsize > 8)
groupsize = 1;
if ((len % groupsize) != 0) /* no mixed size output */
groupsize = 1;
+ if (rowsize % groupsize)
+ rowsize -= rowsize % groupsize;
+
+ if (len > rowsize) /* limit to one line at a time */
+ len = rowsize;
+
ngroups = len / groupsize;
ascii_column = rowsize * 2 + rowsize / groupsize + 1;
@@ -216,7 +219,7 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* 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
+ * @rowsize: number of bytes to print per line; must be a multiple of groupsize
* @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
@@ -226,10 +229,9 @@ EXPORT_SYMBOL(hex_dump_to_buffer);
* 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.,
- * 16 or 32 bytes of input data converted to hex + ASCII output.
* print_hex_dump() iterates over the entire input @buf, breaking it into
- * "line size" chunks to format and print.
+ * lines of rowsize/groupsize groups of input data converted to hex +
+ * (optionally) ASCII output.
*
* E.g.:
* print_hex_dump(KERN_DEBUG, "raw data: ", DUMP_PREFIX_ADDRESS,
@@ -246,17 +248,29 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
{
const u8 *ptr = buf;
int i, linelen, remaining = len;
- unsigned char linebuf[32 * 3 + 2 + 32 + 1];
+ unsigned char *linebuf;
+ unsigned int linebuf_len;
- if (rowsize != 16 && rowsize != 32)
- rowsize = 16;
+ if (rowsize % groupsize)
+ rowsize -= rowsize % groupsize;
+
+ /* Worst case line length:
+ * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in, NULL
+ */
+ linebuf_len = rowsize * 3 + 2 + rowsize + 1;
+ linebuf = kzalloc(linebuf_len, GFP_KERNEL);
+ if (!linebuf) {
+ printk("%s%shexdump: Could not alloc %u bytes for buffer\n",
+ level, prefix_str, linebuf_len);
+ return;
+ }
for (i = 0; i < len; i += rowsize) {
linelen = min(remaining, rowsize);
remaining -= rowsize;
hex_dump_to_buffer(ptr + i, linelen, rowsize, groupsize,
- linebuf, sizeof(linebuf), ascii);
+ linebuf, linebuf_len, ascii);
switch (prefix_type) {
case DUMP_PREFIX_ADDRESS:
@@ -271,6 +285,8 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
break;
}
}
+
+ kfree(linebuf);
}
EXPORT_SYMBOL(print_hex_dump);
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index d78ddd62ffd0..6ab75a209b43 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -14,15 +14,25 @@ static const unsigned char data_b[] = {
'\x70', '\xba', '\xc4', '\x24', '\x7d', '\x83', '\x34', '\x9b', /* 08 - 0f */
'\xa6', '\x9c', '\x31', '\xad', '\x9c', '\x0f', '\xac', '\xe9', /* 10 - 17 */
'\x4c', '\xd1', '\x19', '\x99', '\x43', '\xb1', '\xaf', '\x0c', /* 18 - 1f */
+ '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', /* 20 - 27 */
+ '\x0f', '\x0e', '\x0d', '\x0c', '\x0b', '\x0a', '\x09', '\x08', /* 28 - 2f */
+ '\x10', '\x11', '\x12', '\x13', '\x14', '\x15', '\x16', '\x17', /* 30 - 37 */
+ '\x1f', '\x1e', '\x1d', '\x1c', '\x1b', '\x1a', '\x19', '\x18', /* 38 - 3f */
};
-static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
+static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C..."
+ "................................";
static const char * const test_data_1[] __initconst = {
"be", "32", "db", "7b", "0a", "18", "93", "b2",
"70", "ba", "c4", "24", "7d", "83", "34", "9b",
"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
"4c", "d1", "19", "99", "43", "b1", "af", "0c",
+ "00", "01", "02", "03", "04", "05", "06", "07",
+ "0f", "0e", "0d", "0c", "0b", "0a", "09", "08",
+ "10", "11", "12", "13", "14", "15", "16", "17",
+ "1f", "1e", "1d", "1c", "1b", "1a", "19", "18",
+ NULL
};
static const char * const test_data_2_le[] __initconst = {
@@ -30,6 +40,11 @@ static const char * const test_data_2_le[] __initconst = {
"ba70", "24c4", "837d", "9b34",
"9ca6", "ad31", "0f9c", "e9ac",
"d14c", "9919", "b143", "0caf",
+ "0100", "0302", "0504", "0706",
+ "0e0f", "0c0d", "0a0b", "0809",
+ "1110", "1312", "1514", "1716",
+ "1e1f", "1c1d", "1a1b", "1819",
+ NULL
};
static const char * const test_data_2_be[] __initconst = {
@@ -37,26 +52,43 @@ static const char * const test_data_2_be[] __initconst = {
"70ba", "c424", "7d83", "349b",
"a69c", "31ad", "9c0f", "ace9",
"4cd1", "1999", "43b1", "af0c",
+ "0001", "0203", "0405", "0607",
+ "0f0e", "0d0c", "0b0a", "0908",
+ "1011", "1213", "1415", "1617",
+ "1f1e", "1d1c", "1b1a", "1918",
+ NULL
};
static const char * const test_data_4_le[] __initconst = {
"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
+ "03020100", "07060504", "0c0d0e0f", "08090a0b",
+ "13121110", "17161514", "1c1d1e1f", "18191a1b",
+ NULL
};
static const char * const test_data_4_be[] __initconst = {
"be32db7b", "0a1893b2", "70bac424", "7d83349b",
"a69c31ad", "9c0face9", "4cd11999", "43b1af0c",
+ "00010203", "04050607", "0f0e0d0c", "0b0a0908",
+ "10111213", "14151617", "1f1e1d1c", "1b1a1918",
+ NULL
};
static const char * const test_data_8_le[] __initconst = {
"b293180a7bdb32be", "9b34837d24c4ba70",
"e9ac0f9cad319ca6", "0cafb1439919d14c",
+ "0706050403020100", "08090a0b0c0d0e0f",
+ "1716151413121110", "18191a1b1c1d1e1f",
+ NULL
};
static const char * const test_data_8_be[] __initconst = {
"be32db7b0a1893b2", "70bac4247d83349b",
"a69c31ad9c0face9", "4cd1199943b1af0c",
+ "0001020304050607", "0f0e0d0c0b0a0908",
+ "1011121314151617", "1f1e1d1c1b1a1918",
+ NULL
};
#define FILL_CHAR '#'
@@ -75,9 +107,6 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
unsigned int i;
const bool is_be = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
- if (rs != 16 && rs != 32)
- rs = 16;
-
if (l > rs)
l = rs;
@@ -97,7 +126,12 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
p = test;
for (i = 0; i < l / gs; i++) {
const char *q = *result++;
- size_t amount = strlen(q);
+ size_t amount;
+
+ if (!q)
+ break;
+
+ amount = strlen(q);
memcpy(p, q, amount);
p += amount;
@@ -120,7 +154,7 @@ static void __init test_hexdump_prepare_test(size_t len, int rowsize,
*p = '\0';
}
-#define TEST_HEXDUMP_BUF_SIZE (32 * 3 + 2 + 32 + 1)
+#define TEST_HEXDUMP_BUF_SIZE (64 * 3 + 2 + 64 + 1)
static void __init test_hexdump(size_t len, int rowsize, int groupsize,
bool ascii)
@@ -215,7 +249,7 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
static void __init test_hexdump_overflow_set(size_t buflen, bool ascii)
{
unsigned int i = 0;
- int rs = (get_random_int() % 2 + 1) * 16;
+ int rs = (get_random_int() % 4 + 1) * 16;
do {
int gs = 1 << i;
@@ -230,11 +264,11 @@ static int __init test_hexdump_init(void)
unsigned int i;
int rowsize;
- rowsize = (get_random_int() % 2 + 1) * 16;
+ rowsize = (get_random_int() % 4 + 1) * 16;
for (i = 0; i < 16; i++)
test_hexdump_set(rowsize, false);
- rowsize = (get_random_int() % 2 + 1) * 16;
+ rowsize = (get_random_int() % 4 + 1) * 16;
for (i = 0; i < 16; i++)
test_hexdump_set(rowsize, true);
--
2.21.0
From: Alastair D'Silva <[email protected]>
The overflow tests did not account for the situation where no
overflow occurs and len < rowsize.
This patch renames the cryptic variables and accounts for the
above case.
The selftests now pass.
Signed-off-by: Alastair D'Silva <[email protected]>
---
lib/test_hexdump.c | 47 ++++++++++++++++++++++++++--------------------
1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/lib/test_hexdump.c b/lib/test_hexdump.c
index 5144899d3c6b..d78ddd62ffd0 100644
--- a/lib/test_hexdump.c
+++ b/lib/test_hexdump.c
@@ -163,45 +163,52 @@ static void __init test_hexdump_overflow(size_t buflen, size_t len,
{
char test[TEST_HEXDUMP_BUF_SIZE];
char buf[TEST_HEXDUMP_BUF_SIZE];
- int rs = rowsize, gs = groupsize;
- int ae, he, e, f, r;
- bool a;
+ int ascii_len, hex_len, expected_len, fill_point, ngroups, rc;
+ bool match;
total_tests++;
memset(buf, FILL_CHAR, sizeof(buf));
- r = hex_dump_to_buffer(data_b, len, rs, gs, buf, buflen, ascii);
+ rc = hex_dump_to_buffer(data_b, len, rowsize, groupsize, buf, buflen, ascii);
/*
* Caller must provide the data length multiple of groupsize. The
* calculations below are made with that assumption in mind.
*/
- ae = rs * 2 /* hex */ + rs / gs /* spaces */ + 1 /* space */ + len /* ascii */;
- he = (gs * 2 /* hex */ + 1 /* space */) * len / gs - 1 /* no trailing space */;
+ ngroups = rowsize / groupsize;
+ hex_len = (groupsize * 2 /* hex */ + 1 /* spaces */) * ngroups
+ - 1 /* no trailing space */;
+ ascii_len = hex_len + 2 /* space */ + len /* ascii */;
+
+ if (len < rowsize) {
+ ngroups = len / groupsize;
+ hex_len = (groupsize * 2 /* hex */ + 1 /* spaces */) * ngroups
+ - 1 /* no trailing space */;
+ }
- if (ascii)
- e = ae;
- else
- e = he;
+ expected_len = (ascii) ? ascii_len : hex_len;
- f = min_t(int, e + 1, buflen);
+ fill_point = min_t(int, expected_len + 1, buflen);
if (buflen) {
- test_hexdump_prepare_test(len, rs, gs, test, sizeof(test), ascii);
- test[f - 1] = '\0';
+ test_hexdump_prepare_test(len, rowsize, groupsize, test,
+ sizeof(test), ascii);
+ test[fill_point - 1] = '\0';
}
- memset(test + f, FILL_CHAR, sizeof(test) - f);
+ memset(test + fill_point, FILL_CHAR, sizeof(test) - fill_point);
- a = r == e && !memcmp(test, buf, TEST_HEXDUMP_BUF_SIZE);
+ match = rc == expected_len && !memcmp(test, buf, TEST_HEXDUMP_BUF_SIZE);
buf[sizeof(buf) - 1] = '\0';
- if (!a) {
- pr_err("Len: %zu buflen: %zu strlen: %zu\n",
- len, buflen, strnlen(buf, sizeof(buf)));
- pr_err("Result: %d '%s'\n", r, buf);
- pr_err("Expect: %d '%s'\n", e, test);
+ if (!match) {
+ pr_err("rowsize: %u groupsize: %u ascii: %d Len: %zu buflen: %zu strlen: %zu\n",
+ rowsize, groupsize, ascii, len, buflen,
+ strnlen(buf, sizeof(buf)));
+ pr_err("Result: %d '%-.*s'\n", rc, (int)buflen, buf);
+ pr_err("Expect: %d '%-.*s'\n", expected_len, (int)buflen, test);
failed_tests++;
+
}
}
--
2.21.0
From: Alastair D'Silva <[email protected]>
With the wider display format, it can become hard to identify how many
bytes into the line you are looking at.
The patch adds new flags to hex_dump_to_buffer() and print_hex_dump() to
print vertical lines to separate every N groups of bytes.
eg.
buf:00000000: 454d414e 43415053|4e495f45 00584544 NAMESPAC|E_INDEX.
buf:00000010: 00000000 00000002|00000000 00000000 ........|........
Signed-off-by: Alastair D'Silva <[email protected]>
---
include/linux/printk.h | 3 +++
lib/hexdump.c | 59 ++++++++++++++++++++++++++++++++++++------
2 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 97dd29a2bd77..c6b748f66a82 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -484,6 +484,9 @@ enum {
#define HEXDUMP_ASCII BIT(0)
#define HEXDUMP_SUPPRESS_REPEATED BIT(1)
+#define HEXDUMP_2_GRP_LINES BIT(2)
+#define HEXDUMP_4_GRP_LINES BIT(3)
+#define HEXDUMP_8_GRP_LINES BIT(4)
extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
diff --git a/lib/hexdump.c b/lib/hexdump.c
index 08c6084d7daa..4da7d24826fb 100644
--- a/lib/hexdump.c
+++ b/lib/hexdump.c
@@ -77,6 +77,23 @@ char *bin2hex(char *dst, const void *src, size_t count)
}
EXPORT_SYMBOL(bin2hex);
+static const char *group_separator(int group, u64 flags)
+{
+ if (group == 0)
+ return " ";
+
+ if ((flags & HEXDUMP_8_GRP_LINES) && !((group) % 8))
+ return "|";
+
+ if ((flags & HEXDUMP_4_GRP_LINES) && !((group) % 4))
+ return "|";
+
+ if ((flags & HEXDUMP_2_GRP_LINES) && !((group) % 2))
+ return "|";
+
+ return " ";
+}
+
/**
* hex_dump_to_buffer - convert a blob of data to "hex ASCII" in memory
* @buf: data blob to dump
@@ -87,6 +104,9 @@ EXPORT_SYMBOL(bin2hex);
* @linebuflen: total size of @linebuf, including space for terminating NUL
* @flags: A bitwise OR of the following flags:
* HEXDUMP_ASCII: include ASCII after the hex output
+ * HEXDUMP_2_GRP_LINES: insert a '|' after every 2 groups
+ * HEXDUMP_4_GRP_LINES: insert a '|' after every 4 groups
+ * HEXDUMP_8_GRP_LINES: insert a '|' after every 8 groups
*
* hex_dump_to_buffer() works on one "line" of output at a time, converting
* <groupsize> bytes of input to hexadecimal (and optionally printable ASCII)
@@ -118,6 +138,7 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
int j, lx = 0;
int ascii_column;
int ret;
+ int line_chars = 0;
if (!is_power_of_2(groupsize) || groupsize > 8)
groupsize = 1;
@@ -144,7 +165,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
for (j = 0; j < ngroups; j++) {
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%16.16llx", j ? " " : "",
+ "%s%16.16llx",
+ j ? group_separator(j, flags) : "",
get_unaligned(ptr8 + j));
if (ret >= linebuflen - lx)
goto overflow1;
@@ -155,7 +177,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
for (j = 0; j < ngroups; j++) {
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%8.8x", j ? " " : "",
+ "%s%8.8x",
+ j ? group_separator(j, flags) : "",
get_unaligned(ptr4 + j));
if (ret >= linebuflen - lx)
goto overflow1;
@@ -166,7 +189,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
for (j = 0; j < ngroups; j++) {
ret = snprintf(linebuf + lx, linebuflen - lx,
- "%s%4.4x", j ? " " : "",
+ "%s%4.4x",
+ j ? group_separator(j, flags) : "",
get_unaligned(ptr2 + j));
if (ret >= linebuflen - lx)
goto overflow1;
@@ -196,11 +220,26 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
goto overflow2;
linebuf[lx++] = ' ';
}
+
+ if (flags & HEXDUMP_2_GRP_LINES)
+ line_chars = groupsize * 2;
+ if (flags & HEXDUMP_4_GRP_LINES)
+ line_chars = groupsize * 4;
+ if (flags & HEXDUMP_8_GRP_LINES)
+ line_chars = groupsize * 8;
+
for (j = 0; j < len; j++) {
if (linebuflen < lx + 2)
goto overflow2;
ch = ptr[j];
linebuf[lx++] = (isascii(ch) && isprint(ch)) ? ch : '.';
+
+ if (line_chars && ((j + 1) < len) &&
+ ((j + 1) % line_chars == 0)) {
+ if (linebuflen < lx + 2)
+ goto overflow2;
+ linebuf[lx++] = '|';
+ }
}
nil:
linebuf[lx] = '\0';
@@ -208,7 +247,8 @@ int hex_dump_to_buffer(const void *buf, size_t len, int rowsize, int groupsize,
overflow2:
linebuf[lx++] = '\0';
overflow1:
- return (flags & HEXDUMP_ASCII) ? ascii_column + len :
+ return (flags & HEXDUMP_ASCII) ? ascii_column + len +
+ (len - 1) / line_chars :
(groupsize * 2 + 1) * ngroups - 1;
}
EXPORT_SYMBOL(hex_dump_to_buffer);
@@ -246,7 +286,7 @@ static void announce_skipped(const char *level, const char *prefix_str,
if (count == 0)
return;
- printk("%s%s ** Skipped %lu bytes of value 0x%x **\n",
+ printk("%s%s ** Skipped %lu bytes of value 0x%02x **\n",
level, prefix_str, count, val);
}
@@ -266,6 +306,9 @@ static void announce_skipped(const char *level, const char *prefix_str,
* HEXDUMP_ASCII: include ASCII after the hex output
* HEXDUMP_SUPPRESS_REPEATED: suppress repeated lines of identical
* bytes
+ * HEXDUMP_2_GRP_LINES: insert a '|' after every 2 groups
+ * HEXDUMP_4_GRP_LINES: insert a '|' after every 4 groups
+ * HEXDUMP_8_GRP_LINES: insert a '|' after every 8 groups
*
* Given a buffer of u8 data, print_hex_dump() prints a hex + ASCII dump
* to the kernel log at the specified kernel log level, with an optional
@@ -295,14 +338,14 @@ void print_hex_dump_ext(const char *level, const char *prefix_str,
u8 skipped_val = 0;
size_t skipped = 0;
-
if (rowsize % groupsize)
rowsize -= rowsize % groupsize;
/* Worst case line length:
- * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in, NULL
+ * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in,
+ * 1 char per N groups, NULL
*/
- linebuf_len = rowsize * 3 + 2 + rowsize + 1;
+ linebuf_len = rowsize * 3 + 2 + rowsize + rowsize / groupsize + 1;
linebuf = kzalloc(linebuf_len, GFP_KERNEL);
if (!linebuf) {
printk("%s%shexdump: Could not alloc %u bytes for buffer\n",
--
2.21.0
Hi,
Just a comment style nit below...
On 6/16/19 7:04 PM, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> This patch removes the hardcoded row limits and allows for
> other lengths. These lengths must still be a multiple of
> groupsize.
>
> This allows structs that are not 16/32 bytes to display on
> a single line.
>
> This patch also expands the self-tests to test row sizes
> up to 64 bytes (though they can now be arbitrarily long).
>
> Signed-off-by: Alastair D'Silva <[email protected]>
> ---
> lib/hexdump.c | 48 ++++++++++++++++++++++++++++--------------
> lib/test_hexdump.c | 52 ++++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 75 insertions(+), 25 deletions(-)
>
> diff --git a/lib/hexdump.c b/lib/hexdump.c
> index 81b70ed37209..3943507bc0e9 100644
> --- a/lib/hexdump.c
> +++ b/lib/hexdump.c
> @@ -246,17 +248,29 @@ void print_hex_dump(const char *level, const char *prefix_str, int prefix_type,
> {
> const u8 *ptr = buf;
> int i, linelen, remaining = len;
> - unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> + unsigned char *linebuf;
> + unsigned int linebuf_len;
>
> - if (rowsize != 16 && rowsize != 32)
> - rowsize = 16;
> + if (rowsize % groupsize)
> + rowsize -= rowsize % groupsize;
> +
> + /* Worst case line length:
> + * 2 hex chars + space per byte in, 2 spaces, 1 char per byte in, NULL
> + */
According to Documentation/process/coding-style.rst:
The preferred style for long (multi-line) comments is:
.. code-block:: c
/*
* This is the preferred style for multi-line
* comments in the Linux kernel source code.
* Please use it consistently.
*
* Description: A column of asterisks on the left side,
* with beginning and ending almost-blank lines.
*/
except in networking software.
> + linebuf_len = rowsize * 3 + 2 + rowsize + 1;
> + linebuf = kzalloc(linebuf_len, GFP_KERNEL);
> + if (!linebuf) {
> + printk("%s%shexdump: Could not alloc %u bytes for buffer\n",
> + level, prefix_str, linebuf_len);
> + return;
> + }
--
~Randy
On Mon, 2019-06-17 at 15:47 -0700, Randy Dunlap wrote:
> Hi,
> Just a comment style nit below...
>
> On 6/16/19 7:04 PM, Alastair D'Silva wrote:
> > From: Alastair D'Silva <[email protected]>
> >
> > This patch removes the hardcoded row limits and allows for
> > other lengths. These lengths must still be a multiple of
> > groupsize.
> >
> > This allows structs that are not 16/32 bytes to display on
> > a single line.
> >
> > This patch also expands the self-tests to test row sizes
> > up to 64 bytes (though they can now be arbitrarily long).
> >
> > Signed-off-by: Alastair D'Silva <[email protected]>
> > ---
> > lib/hexdump.c | 48 ++++++++++++++++++++++++++++--------------
> > lib/test_hexdump.c | 52 ++++++++++++++++++++++++++++++++++++++--
> > ------
> > 2 files changed, 75 insertions(+), 25 deletions(-)
> >
> > diff --git a/lib/hexdump.c b/lib/hexdump.c
> > index 81b70ed37209..3943507bc0e9 100644
> > --- a/lib/hexdump.c
> > +++ b/lib/hexdump.c
> > @@ -246,17 +248,29 @@ void print_hex_dump(const char *level, const
> > char *prefix_str, int prefix_type,
> > {
> > const u8 *ptr = buf;
> > int i, linelen, remaining = len;
> > - unsigned char linebuf[32 * 3 + 2 + 32 + 1];
> > + unsigned char *linebuf;
> > + unsigned int linebuf_len;
> >
> > - if (rowsize != 16 && rowsize != 32)
> > - rowsize = 16;
> > + if (rowsize % groupsize)
> > + rowsize -= rowsize % groupsize;
> > +
> > + /* Worst case line length:
> > + * 2 hex chars + space per byte in, 2 spaces, 1 char per byte
> > in, NULL
> > + */
>
> According to Documentation/process/coding-style.rst:
>
> The preferred style for long (multi-line) comments is:
>
> .. code-block:: c
>
> /*
> * This is the preferred style for multi-line
> * comments in the Linux kernel source code.
> * Please use it consistently.
> *
> * Description: A column of asterisks on the left side,
> * with beginning and ending almost-blank lines.
> */
>
Thanks Randy, I'll address this.
--
Alastair D'Silva mob: 0423 762 819
skype: alastair_dsilva
Twitter: @EvilDeece
blog: http://alastair.d-silva.org
On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> From: Alastair D'Silva <[email protected]>
>
> Apologies for the large CC list, it's a heads up for those responsible
> for subsystems where a prototype change in generic code causes a change
> in those subsystems.
>
> This series enhances hexdump.
Still not a fan of these patches.
> These improve the readability of the dumped data in certain situations
> (eg. wide terminals are available, many lines of empty bytes exist, etc).
Changing hexdump's last argument from bool to int is odd.
Perhaps a new function should be added instead of changing
the existing hexdump.
> The default behaviour of hexdump is unchanged, however, the prototype
> for hex_dump_to_buffer() has changed, and print_hex_dump() has been
> renamed to print_hex_dump_ext(), with a wrapper replacing it for
> compatibility with existing code, which would have been too invasive to
> change.
>
> Hexdump selftests have be run & confirmed passed.
On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > From: Alastair D'Silva <[email protected]>
> >
> > Apologies for the large CC list, it's a heads up for those
> > responsible
> > for subsystems where a prototype change in generic code causes a
> > change
> > in those subsystems.
> >
> > This series enhances hexdump.
>
> Still not a fan of these patches.
I'm afraid there's not too much action I can take on that, I'm happy to
address specific issues though.
>
> > These improve the readability of the dumped data in certain
> > situations
> > (eg. wide terminals are available, many lines of empty bytes exist,
> > etc).
>
> Changing hexdump's last argument from bool to int is odd.
>
Think of it as replacing a single boolean with many booleans.
> Perhaps a new function should be added instead of changing
> the existing hexdump.
>
There's only a handful of consumers, I don't think there is a value-add
in creating more wrappers vs updating the existing callers.
--
Alastair D'Silva mob: 0423 762 819
skype: alastair_dsilva
Twitter: @EvilDeece
blog: http://alastair.d-silva.org
On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
> On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
> > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > > > From: Alastair D'Silva <[email protected]>
> > > >
> > > > Apologies for the large CC list, it's a heads up for those
> > > > responsible
> > > > for subsystems where a prototype change in generic code causes
> > > > a
> > > > change
> > > > in those subsystems.
> > > >
> > > > This series enhances hexdump.
> > >
> > > Still not a fan of these patches.
> >
> > I'm afraid there's not too much action I can take on that, I'm
> > happy to
> > address specific issues though.
> >
> > > > These improve the readability of the dumped data in certain
> > > > situations
> > > > (eg. wide terminals are available, many lines of empty bytes
> > > > exist,
> > > > etc).
>
> I think it's generally overkill for the desired uses.
I understand where you're coming from, however, these patches make it a
lot easier to work with large chucks of binary data. I think it makes
more sense to have these patches upstream, even though committed code
may not necessarily have all the features enabled, as it means that
devs won't have to apply out-of-tree patches during development to make
larger dumps manageable.
>
> > > Changing hexdump's last argument from bool to int is odd.
> > >
> >
> > Think of it as replacing a single boolean with many booleans.
>
> I understand it. It's odd.
>
> I would rather not have a mixture of true, false, and apparently
> random collections of bitfields like 0xd or 0b1011 or their
> equivalent or'd defines.
>
Where's the mixture? What would you propose instead?
--
Alastair D'Silva mob: 0423 762 819
skype: alastair_dsilva
Twitter: @EvilDeece
blog: http://alastair.d-silva.org
On Thu, 2019-06-20 at 11:14 +1000, Alastair D'Silva wrote:
> On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
> > On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
> > > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
> > > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
> > > > > From: Alastair D'Silva <[email protected]>
> > > > >
> > > > > Apologies for the large CC list, it's a heads up for those
> > > > > responsible
> > > > > for subsystems where a prototype change in generic code causes
> > > > > a
> > > > > change
> > > > > in those subsystems.
> > > > >
> > > > > This series enhances hexdump.
> > > >
> > > > Still not a fan of these patches.
> > >
> > > I'm afraid there's not too much action I can take on that, I'm
> > > happy to
> > > address specific issues though.
> > >
> > > > > These improve the readability of the dumped data in certain
> > > > > situations
> > > > > (eg. wide terminals are available, many lines of empty bytes
> > > > > exist,
> > > > > etc).
> >
> > I think it's generally overkill for the desired uses.
>
> I understand where you're coming from, however, these patches make it a
> lot easier to work with large chucks of binary data. I think it makes
> more sense to have these patches upstream, even though committed code
> may not necessarily have all the features enabled, as it means that
> devs won't have to apply out-of-tree patches during development to make
> larger dumps manageable.
>
> > > > Changing hexdump's last argument from bool to int is odd.
> > > >
> > >
> > > Think of it as replacing a single boolean with many booleans.
> >
> > I understand it. It's odd.
> >
> > I would rather not have a mixture of true, false, and apparently
> > random collections of bitfields like 0xd or 0b1011 or their
> > equivalent or'd defines.
> >
>
> Where's the mixture? What would you propose instead?
create a hex_dump_to_buffer_ext with a new argument
and a new static inline for the old hex_dump_to_buffer
without modifying the argument list that calls
hex_dump_to_buffer with whatever added argument content
you need.
Something like:
static inline
int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
int groupsize, char *linebuf, size_t linebuflen,
bool ascii)
{
return hex_dump_to_buffer_ext(buf, len, rowsize, groupsize,
linebuf, linebuflen, ascii, 0);
}
and remove EXPORT_SYMBOL(hex_dump_to_buffer)
On Wed, 19 Jun 2019, Joe Perches <[email protected]> wrote:
> On Thu, 2019-06-20 at 11:14 +1000, Alastair D'Silva wrote:
>> On Wed, 2019-06-19 at 17:35 -0700, Joe Perches wrote:
>> > On Thu, 2019-06-20 at 09:15 +1000, Alastair D'Silva wrote:
>> > > On Wed, 2019-06-19 at 09:31 -0700, Joe Perches wrote:
>> > > > On Mon, 2019-06-17 at 12:04 +1000, Alastair D'Silva wrote:
>> > > > > From: Alastair D'Silva <[email protected]>
>> > > > >
>> > > > > Apologies for the large CC list, it's a heads up for those
>> > > > > responsible
>> > > > > for subsystems where a prototype change in generic code causes
>> > > > > a
>> > > > > change
>> > > > > in those subsystems.
>> > > > >
>> > > > > This series enhances hexdump.
>> > > >
>> > > > Still not a fan of these patches.
>> > >
>> > > I'm afraid there's not too much action I can take on that, I'm
>> > > happy to
>> > > address specific issues though.
>> > >
>> > > > > These improve the readability of the dumped data in certain
>> > > > > situations
>> > > > > (eg. wide terminals are available, many lines of empty bytes
>> > > > > exist,
>> > > > > etc).
>> >
>> > I think it's generally overkill for the desired uses.
>>
>> I understand where you're coming from, however, these patches make it a
>> lot easier to work with large chucks of binary data. I think it makes
>> more sense to have these patches upstream, even though committed code
>> may not necessarily have all the features enabled, as it means that
>> devs won't have to apply out-of-tree patches during development to make
>> larger dumps manageable.
>>
>> > > > Changing hexdump's last argument from bool to int is odd.
>> > > >
>> > >
>> > > Think of it as replacing a single boolean with many booleans.
>> >
>> > I understand it. It's odd.
>> >
>> > I would rather not have a mixture of true, false, and apparently
>> > random collections of bitfields like 0xd or 0b1011 or their
>> > equivalent or'd defines.
>> >
>>
>> Where's the mixture? What would you propose instead?
>
> create a hex_dump_to_buffer_ext with a new argument
> and a new static inline for the old hex_dump_to_buffer
> without modifying the argument list that calls
> hex_dump_to_buffer with whatever added argument content
> you need.
>
> Something like:
>
> static inline
> int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> int groupsize, char *linebuf, size_t linebuflen,
> bool ascii)
> {
> return hex_dump_to_buffer_ext(buf, len, rowsize, groupsize,
> linebuf, linebuflen, ascii, 0);
> }
>
> and remove EXPORT_SYMBOL(hex_dump_to_buffer)
If you decide to do something like this, I'd actually suggest you drop
the bool ascii parameter from hex_dump_to_buffer() altogether, and
replace the callers that do require ascii with
hex_dump_to_buffer_ext(..., HEXDUMP_ASCII). Even if that also requires
touching all callers.
But no strong opinions, really.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center