[PATCH v2 0/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[PATCH v2 0/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Vladimir Sementsov-Ogievskiy]

Hi all. qemu_hexdump() wrongly indents ASCII part of the output for
the list line, it it's not bound to 16-bytes boundary. Let's fix.

v2: add test

Vladimir Sementsov-Ogievskiy (2):
  util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic
  tests/unit: add unit test for qemu_hexdump()

 tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
 util/hexdump.c           | 38 ++++++++++++++++++++++-------------
 2 files changed, 67 insertions(+), 14 deletions(-)

-- 
2.48.1

[PATCH v2 1/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Vladimir Sementsov-Ogievskiy]

QEMU_HEXDUMP_LINE_WIDTH calculation doesn't correspond to
qemu_hexdump_line(). This leads to last line of the dump (when
length is not multiply of 16) has badly aligned ASCII part.

Let's calculate length the same way.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 util/hexdump.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/util/hexdump.c b/util/hexdump.c
index f29ffceb74..7cfc547261 100644
--- a/util/hexdump.c
+++ b/util/hexdump.c
@@ -22,6 +22,19 @@ static inline char hexdump_nibble(unsigned x)
     return (x < 10 ? '0' : 'a' - 10) + x;
 }
 
+static size_t hexdump_line_length(size_t buf_len, size_t unit_len,
+                                  size_t block_len)
+{
+    size_t est = buf_len * 2;
+    if (unit_len) {
+        est += buf_len / unit_len;
+    }
+    if (block_len) {
+        est += buf_len / block_len;
+    }
+    return est;
+}
+
 GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len,
                            size_t unit_len, size_t block_len)
 {
@@ -30,14 +43,8 @@ GString *qemu_hexdump_line(GString *str, const void *vbuf, size_t len,
 
     if (str == NULL) {
         /* Estimate the length of the output to avoid reallocs. */
-        size_t est = len * 2;
-        if (unit_len) {
-            est += len / unit_len;
-        }
-        if (block_len) {
-            est += len / block_len;
-        }
-        str = g_string_sized_new(est + 1);
+        str = g_string_sized_new(hexdump_line_length(len, unit_len, block_len)
+                                 + 1);
     }
 
     for (u = 0, b = 0; len; u++, b++, len--, buf++) {
@@ -76,13 +83,16 @@ static void asciidump_line(char *line, const void *bufptr, size_t len)
 }
 
 #define QEMU_HEXDUMP_LINE_BYTES 16
-#define QEMU_HEXDUMP_LINE_WIDTH \
-    (QEMU_HEXDUMP_LINE_BYTES * 2 + QEMU_HEXDUMP_LINE_BYTES / 4)
+#define QEMU_HEXDUMP_UNIT 1
+#define QEMU_HEXDUMP_BLOCK 4
 
 void qemu_hexdump(FILE *fp, const char *prefix,
                   const void *bufptr, size_t size)
 {
-    g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
+    int width = hexdump_line_length(QEMU_HEXDUMP_LINE_BYTES,
+                                    QEMU_HEXDUMP_UNIT,
+                                    QEMU_HEXDUMP_BLOCK);
+    g_autoptr(GString) str = g_string_sized_new(width + 1);
     char ascii[QEMU_HEXDUMP_LINE_BYTES + 1];
     size_t b, len;
 
@@ -90,11 +100,11 @@ void qemu_hexdump(FILE *fp, const char *prefix,
         len = MIN(size - b, QEMU_HEXDUMP_LINE_BYTES);
 
         g_string_truncate(str, 0);
-        qemu_hexdump_line(str, bufptr + b, len, 1, 4);
+        qemu_hexdump_line(str, bufptr + b, len,
+                          QEMU_HEXDUMP_UNIT, QEMU_HEXDUMP_BLOCK);
         asciidump_line(ascii, bufptr + b, len);
 
-        fprintf(fp, "%s: %04zx: %-*s %s\n",
-                prefix, b, QEMU_HEXDUMP_LINE_WIDTH, str->str, ascii);
+        fprintf(fp, "%s: %04zx: %-*s %s\n", prefix, b, width, str->str, ascii);
     }
 
 }
-- 
2.48.1

[PATCH v2 2/2] tests/unit: add unit test for qemu_hexdump()

[Vladimir Sementsov-Ogievskiy]

Test, that fix in previous commit make sense.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
index 227acc5995..24fef16a7f 100644
--- a/tests/unit/test-cutils.c
+++ b/tests/unit/test-cutils.c
@@ -3626,6 +3626,44 @@ static void test_si_prefix(void)
     g_assert_cmpstr(si_prefix(18), ==, "E");
 }
 
+static void test_qemu_hexdump_alignment(void)
+{
+    /*
+     * Test that ASCII part is properly aligned for incomplete lines.
+     * This test catches the bug that was fixed in previous commit
+     * "util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic".
+     *
+     * We use data that is not aligned to 16 bytes, so last line
+     * is incomplete.
+     */
+    const uint8_t data[] = {
+        /* First line: 16 bytes */
+        0x48, 0x65, 0x6c, 0x6c, 0x6f, 0x20, 0x57, 0x6f,  /* "Hello Wo" */
+        0x72, 0x6c, 0x64, 0x21, 0x20, 0x54, 0x68, 0x69,  /* "rld! Thi" */
+        /* Second line: 5 bytes (incomplete) */
+        0x73, 0x20, 0x69, 0x73, 0x20                     /* "s is " */
+    };
+    char *output = NULL;
+    size_t size;
+    FILE *stream = open_memstream(&output, &size);
+
+    g_assert_nonnull(stream);
+
+    qemu_hexdump(stream, "test", data, sizeof(data));
+    fclose(stream);
+
+    g_assert_nonnull(output);
+
+    /* We expect proper alignment of "s is" part on the second line */
+    const char *expected =
+        "test: 0000: 48 65 6c 6c  6f 20 57 6f  72 6c 64 21  20 54 68 69   Hello World! Thi\n"
+        "test: 0010: 73 20 69 73  20                                      s is \n";
+
+    g_assert_cmpstr(output, ==, expected);
+
+    free(output);
+}
+
 int main(int argc, char **argv)
 {
     g_test_init(&argc, &argv, NULL);
@@ -3995,5 +4033,10 @@ int main(int argc, char **argv)
                     test_iec_binary_prefix);
     g_test_add_func("/cutils/si_prefix",
                     test_si_prefix);
+
+    /* qemu_hexdump() test */
+    g_test_add_func("/cutils/qemu_hexdump/alignment",
+                    test_qemu_hexdump_alignment);
+
     return g_test_run();
 }
-- 
2.48.1

Re: [PATCH v2 1/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Philippe Mathieu-Daudé]

On 31/10/25 20:02, Vladimir Sementsov-Ogievskiy wrote:
> QEMU_HEXDUMP_LINE_WIDTH calculation doesn't correspond to
> qemu_hexdump_line(). This leads to last line of the dump (when
> length is not multiply of 16) has badly aligned ASCII part.
> 
> Let's calculate length the same way.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   util/hexdump.c | 38 ++++++++++++++++++++++++--------------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/util/hexdump.c b/util/hexdump.c
> index f29ffceb74..7cfc547261 100644
> --- a/util/hexdump.c
> +++ b/util/hexdump.c
> @@ -22,6 +22,19 @@ static inline char hexdump_nibble(unsigned x)
>       return (x < 10 ? '0' : 'a' - 10) + x;
>   }
>   
> +static size_t hexdump_line_length(size_t buf_len, size_t unit_len,
> +                                  size_t block_len)
> +{
> +    size_t est = buf_len * 2;
> +    if (unit_len) {
> +        est += buf_len / unit_len;
> +    }
> +    if (block_len) {
> +        est += buf_len / block_len;
> +    }
> +    return est;
> +}


>   void qemu_hexdump(FILE *fp, const char *prefix,
>                     const void *bufptr, size_t size)
>   {
> -    g_autoptr(GString) str = g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
> +    int width = hexdump_line_length(QEMU_HEXDUMP_LINE_BYTES,

size_t

> +                                    QEMU_HEXDUMP_UNIT,
> +                                    QEMU_HEXDUMP_BLOCK);
> +    g_autoptr(GString) str = g_string_sized_new(width + 1);

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 2/2] tests/unit: add unit test for qemu_hexdump()

[Philippe Mathieu-Daudé]

On 31/10/25 20:02, Vladimir Sementsov-Ogievskiy wrote:
> Test, that fix in previous commit make sense.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 0/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Philippe Mathieu-Daudé]

On 31/10/25 20:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi all. qemu_hexdump() wrongly indents ASCII part of the output for
> the list line, it it's not bound to 16-bytes boundary. Let's fix.
> 
> v2: add test
> 
> Vladimir Sementsov-Ogievskiy (2):
>    util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic
>    tests/unit: add unit test for qemu_hexdump()

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 1/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Philippe Mathieu-Daudé]

On 31/10/25 21:54, Philippe Mathieu-Daudé wrote:
> On 31/10/25 20:02, Vladimir Sementsov-Ogievskiy wrote:
>> QEMU_HEXDUMP_LINE_WIDTH calculation doesn't correspond to
>> qemu_hexdump_line(). This leads to last line of the dump (when
>> length is not multiply of 16) has badly aligned ASCII part.
>>
>> Let's calculate length the same way.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   util/hexdump.c | 38 ++++++++++++++++++++++++--------------
>>   1 file changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/util/hexdump.c b/util/hexdump.c
>> index f29ffceb74..7cfc547261 100644
>> --- a/util/hexdump.c
>> +++ b/util/hexdump.c
>> @@ -22,6 +22,19 @@ static inline char hexdump_nibble(unsigned x)
>>       return (x < 10 ? '0' : 'a' - 10) + x;
>>   }
>> +static size_t hexdump_line_length(size_t buf_len, size_t unit_len,
>> +                                  size_t block_len)
>> +{
>> +    size_t est = buf_len * 2;
>> +    if (unit_len) {
>> +        est += buf_len / unit_len;
>> +    }
>> +    if (block_len) {
>> +        est += buf_len / block_len;
>> +    }
>> +    return est;
>> +}
> 
> 
>>   void qemu_hexdump(FILE *fp, const char *prefix,
>>                     const void *bufptr, size_t size)
>>   {
>> -    g_autoptr(GString) str = 
>> g_string_sized_new(QEMU_HEXDUMP_LINE_WIDTH + 1);
>> +    int width = hexdump_line_length(QEMU_HEXDUMP_LINE_BYTES,
> 
> size_t

Hmm I now see why you use 'int':

../../util/hexdump.c:107:36: error: field width should have type 'int', 
but argument has type 'size_t' (aka 'unsigned long') [-Werror,-Wformat]
   107 |         fprintf(fp, "%s: %04zx: %-*s %s\n", prefix, b, width, 
str->str, ascii);
       |                                 ~~~^                   ~~~~~

> 
>> +                                    QEMU_HEXDUMP_UNIT,
>> +                                    QEMU_HEXDUMP_BLOCK);
>> +    g_autoptr(GString) str = g_string_sized_new(width + 1);
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Re: [PATCH v2 0/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Philippe Mathieu-Daudé]

On 31/10/25 21:56, Philippe Mathieu-Daudé wrote:
> On 31/10/25 20:02, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all. qemu_hexdump() wrongly indents ASCII part of the output for
>> the list line, it it's not bound to 16-bytes boundary. Let's fix.
>>
>> v2: add test
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>    util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic
>>    tests/unit: add unit test for qemu_hexdump()
> 
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

And queued.

Re: [PATCH v2 0/2] util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic

[Vladimir Sementsov-Ogievskiy]

On 01.11.25 00:11, Philippe Mathieu-Daudé wrote:
> On 31/10/25 21:56, Philippe Mathieu-Daudé wrote:
>> On 31/10/25 20:02, Vladimir Sementsov-Ogievskiy wrote:
>>> Hi all. qemu_hexdump() wrongly indents ASCII part of the output for
>>> the list line, it it's not bound to 16-bytes boundary. Let's fix.
>>>
>>> v2: add test
>>>
>>> Vladimir Sementsov-Ogievskiy (2):
>>>    util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic
>>>    tests/unit: add unit test for qemu_hexdump()
>>
>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> And queued.

Thank you!

-- 
Best regards,
Vladimir