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

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

[Vladimir Sementsov-Ogievskiy]

Test, that fix in previous commit make sense.

To not break compilation when we build without
'block', move hexdump.c out of "if have_block"
in meson.build.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---

v3: change meson.build to compile hexdump.c always

 tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
 util/meson.build         |  2 +-
 2 files changed, 44 insertions(+), 1 deletion(-)

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();
 }
diff --git a/util/meson.build b/util/meson.build
index 35029380a3..376f3a6591 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -36,6 +36,7 @@ endif
 util_ss.add(files('defer-call.c'))
 util_ss.add(files('envlist.c', 'path.c', 'module.c'))
 util_ss.add(files('event.c'))
+util_ss.add(files('hexdump.c'))
 util_ss.add(files('host-utils.c'))
 util_ss.add(files('bitmap.c', 'bitops.c'))
 util_ss.add(files('fifo8.c'))
@@ -96,7 +97,6 @@ if have_block
   util_ss.add(files('buffer.c'))
   util_ss.add(files('bufferiszero.c'))
   util_ss.add(files('hbitmap.c'))
-  util_ss.add(files('hexdump.c'))
   util_ss.add(files('iova-tree.c'))
   util_ss.add(files('iov.c'))
   util_ss.add(files('nvdimm-utils.c'))
-- 
2.48.1

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

[Daniel P. Berrangé]

On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Test, that fix in previous commit make sense.
> 
> To not break compilation when we build without
> 'block', move hexdump.c out of "if have_block"
> in meson.build.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> v3: change meson.build to compile hexdump.c always
> 
>  tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
>  util/meson.build         |  2 +-
>  2 files changed, 44 insertions(+), 1 deletion(-)

> +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;

Could be  g_autofree, and avoid the later 'free()' call.

> +    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);
> +}

The above comment is trivial, so

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Vladimir Sementsov-Ogievskiy]

On 13.11.25 12:10, Daniel P. Berrangé wrote:
> On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Test, that fix in previous commit make sense.
>>
>> To not break compilation when we build without
>> 'block', move hexdump.c out of "if have_block"
>> in meson.build.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>
>> v3: change meson.build to compile hexdump.c always
>>
>>   tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
>>   util/meson.build         |  2 +-
>>   2 files changed, 44 insertions(+), 1 deletion(-)
> 
>> +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;
> 
> Could be  g_autofree, and avoid the later 'free()' call.

I'm not sure that it's correct to replace free() by g_free()..

Documentation says "bad things can happen" https://docs.gtk.org/glib/memory.html

> 
>> +    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);
>> +}
> 
> The above comment is trivial, so
> 
>    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> 
> With regards,
> Daniel


-- 
Best regards,
Vladimir

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

[Daniel P. Berrangé]

On Thu, Nov 13, 2025 at 01:20:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 13.11.25 12:10, Daniel P. Berrangé wrote:
> > On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Test, that fix in previous commit make sense.
> > > 
> > > To not break compilation when we build without
> > > 'block', move hexdump.c out of "if have_block"
> > > in meson.build.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > > 
> > > v3: change meson.build to compile hexdump.c always
> > > 
> > >   tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
> > >   util/meson.build         |  2 +-
> > >   2 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > > +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;
> > 
> > Could be  g_autofree, and avoid the later 'free()' call.
> 
> I'm not sure that it's correct to replace free() by g_free()..
> 
> Documentation says "bad things can happen" https://docs.gtk.org/glib/memory.html

Note where it says:

  "Since GLib 2.46, g_malloc() is hardcoded to always use the system
   malloc implementation."

I added that guarantee to glib docs specifically so apps no longer
have to match free with g_free.  You should still not mix up the
C free vs C++ delete, or  free vs g_slice_free, but that's not an
issue for QEMU.

> 
> > 
> > > +    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);
> > > +}
> > 
> > The above comment is trivial, so
> > 
> >    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > 
> > 
> > With regards,
> > Daniel
> 
> 
> -- 
> Best regards,
> Vladimir
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Vladimir Sementsov-Ogievskiy]

On 13.11.25 13:33, Daniel P. Berrangé wrote:
> On Thu, Nov 13, 2025 at 01:20:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 13.11.25 12:10, Daniel P. Berrangé wrote:
>>> On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Test, that fix in previous commit make sense.
>>>>
>>>> To not break compilation when we build without
>>>> 'block', move hexdump.c out of "if have_block"
>>>> in meson.build.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>> ---
>>>>
>>>> v3: change meson.build to compile hexdump.c always
>>>>
>>>>    tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
>>>>    util/meson.build         |  2 +-
>>>>    2 files changed, 44 insertions(+), 1 deletion(-)
>>>
>>>> +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;
>>>
>>> Could be  g_autofree, and avoid the later 'free()' call.
>>
>> I'm not sure that it's correct to replace free() by g_free()..
>>
>> Documentation says "bad things can happen" https://docs.gtk.org/glib/memory.html
> 
> Note where it says:
> 
>    "Since GLib 2.46, g_malloc() is hardcoded to always use the system
>     malloc implementation."
> > I added that guarantee to glib docs specifically so apps no longer
> have to match free with g_free.  You should still not mix up the
> C free vs C++ delete, or  free vs g_slice_free, but that's not an
> issue for QEMU.
> 

O, that's cool. Still the previous paragraph better be adjusted too, to match
this last sentence. Otherwise they are in contradiction. I'll send v4.

>>
>>>
>>>> +    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);
>>>> +}
>>>
>>> The above comment is trivial, so
>>>
>>>     Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>>>
>>>
>>>
>>> With regards,
>>> Daniel
>>
>>
>> -- 
>> Best regards,
>> Vladimir
>>
> 
> With regards,
> Daniel


-- 
Best regards,
Vladimir

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

[Peter Maydell]

On Thu, 13 Nov 2025 at 10:33, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Nov 13, 2025 at 01:20:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 13.11.25 12:10, Daniel P. Berrangé wrote:
> > > On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > Test, that fix in previous commit make sense.
> > > >
> > > > To not break compilation when we build without
> > > > 'block', move hexdump.c out of "if have_block"
> > > > in meson.build.
> > > >
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > ---
> > > >
> > > > v3: change meson.build to compile hexdump.c always
> > > >
> > > >   tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
> > > >   util/meson.build         |  2 +-
> > > >   2 files changed, 44 insertions(+), 1 deletion(-)
> > >
> > > > +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;
> > >
> > > Could be  g_autofree, and avoid the later 'free()' call.
> >
> > I'm not sure that it's correct to replace free() by g_free()..
> >
> > Documentation says "bad things can happen" https://docs.gtk.org/glib/memory.html
>
> Note where it says:
>
>   "Since GLib 2.46, g_malloc() is hardcoded to always use the system
>    malloc implementation."
>
> I added that guarantee to glib docs specifically so apps no longer
> have to match free with g_free.  You should still not mix up the
> C free vs C++ delete, or  free vs g_slice_free, but that's not an
> issue for QEMU.

I think for this specific case (the buffer allocated by
open_memstream()) it's probably better to use explicit
free(), because the criterion for "when is it OK to free
this?" is not "when the pointer goes out of scope" but
"when we have called fclose() on the stream". Auto-freeing
the buffer by returning without closing the file would
be a bug.

-- PMM

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

[Daniel P. Berrangé]

On Thu, Nov 13, 2025 at 10:46:42AM +0000, Peter Maydell wrote:
> On Thu, 13 Nov 2025 at 10:33, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Nov 13, 2025 at 01:20:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 13.11.25 12:10, Daniel P. Berrangé wrote:
> > > > On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > Test, that fix in previous commit make sense.
> > > > >
> > > > > To not break compilation when we build without
> > > > > 'block', move hexdump.c out of "if have_block"
> > > > > in meson.build.
> > > > >
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > > ---
> > > > >
> > > > > v3: change meson.build to compile hexdump.c always
> > > > >
> > > > >   tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
> > > > >   util/meson.build         |  2 +-
> > > > >   2 files changed, 44 insertions(+), 1 deletion(-)
> > > >
> > > > > +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;
> > > >
> > > > Could be  g_autofree, and avoid the later 'free()' call.
> > >
> > > I'm not sure that it's correct to replace free() by g_free()..
> > >
> > > Documentation says "bad things can happen" https://docs.gtk.org/glib/memory.html
> >
> > Note where it says:
> >
> >   "Since GLib 2.46, g_malloc() is hardcoded to always use the system
> >    malloc implementation."
> >
> > I added that guarantee to glib docs specifically so apps no longer
> > have to match free with g_free.  You should still not mix up the
> > C free vs C++ delete, or  free vs g_slice_free, but that's not an
> > issue for QEMU.
> 
> I think for this specific case (the buffer allocated by
> open_memstream()) it's probably better to use explicit
> free(), because the criterion for "when is it OK to free
> this?" is not "when the pointer goes out of scope" but
> "when we have called fclose() on the stream". Auto-freeing
> the buffer by returning without closing the file would
> be a bug.

Oh good point, lets just leave this as-is.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Vladimir Sementsov-Ogievskiy]

On 13.11.25 14:35, Daniel P. Berrangé wrote:
> On Thu, Nov 13, 2025 at 10:46:42AM +0000, Peter Maydell wrote:
>> On Thu, 13 Nov 2025 at 10:33, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>> On Thu, Nov 13, 2025 at 01:20:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 13.11.25 12:10, Daniel P. Berrangé wrote:
>>>>> On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Test, that fix in previous commit make sense.
>>>>>>
>>>>>> To not break compilation when we build without
>>>>>> 'block', move hexdump.c out of "if have_block"
>>>>>> in meson.build.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>>>> ---
>>>>>>
>>>>>> v3: change meson.build to compile hexdump.c always
>>>>>>
>>>>>>    tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
>>>>>>    util/meson.build         |  2 +-
>>>>>>    2 files changed, 44 insertions(+), 1 deletion(-)
>>>>>
>>>>>> +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;
>>>>>
>>>>> Could be  g_autofree, and avoid the later 'free()' call.
>>>>
>>>> I'm not sure that it's correct to replace free() by g_free()..
>>>>
>>>> Documentation says "bad things can happen" https://docs.gtk.org/glib/memory.html
>>>
>>> Note where it says:
>>>
>>>    "Since GLib 2.46, g_malloc() is hardcoded to always use the system
>>>     malloc implementation."
>>>
>>> I added that guarantee to glib docs specifically so apps no longer
>>> have to match free with g_free.  You should still not mix up the
>>> C free vs C++ delete, or  free vs g_slice_free, but that's not an
>>> issue for QEMU.
>>
>> I think for this specific case (the buffer allocated by
>> open_memstream()) it's probably better to use explicit
>> free(), because the criterion for "when is it OK to free
>> this?" is not "when the pointer goes out of scope" but
>> "when we have called fclose() on the stream". Auto-freeing
>> the buffer by returning without closing the file would
>> be a bug.
> 
> Oh good point, lets just leave this as-is.
> 
> 

Ok.



-- 
Best regards,
Vladimir

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

[Eric Blake]

On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Test, that fix in previous commit make sense.

"previous commit" is ambiguous, when this patch is not part of a
larger series. Maybe use the term "recent commit" and call out its
title and/or commit hash?

> +++ 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".

Ah - that's the line I was expecting in the commit message; and I
found commit 20aa05ed from there.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Vladimir Sementsov-Ogievskiy]

On 13.11.25 16:56, Eric Blake wrote:
> On Thu, Nov 13, 2025 at 09:49:35AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Test, that fix in previous commit make sense.
> 
> "previous commit" is ambiguous, when this patch is not part of a
> larger series. Maybe use the term "recent commit" and call out its
> title and/or commit hash?

Good caught! So we need to rewrite first line into

Test for the  fix in recent commit
  20aa05ed "util/hexdump: fix QEMU_HEXDUMP_LINE_WIDTH logic"


> 
>> +++ 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".
> 
> Ah - that's the line I was expecting in the commit message; and I
> found commit 20aa05ed from there.
> 
> 


-- 
Best regards,
Vladimir

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

[Philippe Mathieu-Daudé]

On 13/11/25 07:49, Vladimir Sementsov-Ogievskiy wrote:
> Test, that fix in previous commit make sense.
> 
> To not break compilation when we build without
> 'block', move hexdump.c out of "if have_block"
> in meson.build.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> 
> v3: change meson.build to compile hexdump.c always
> 
>   tests/unit/test-cutils.c | 43 ++++++++++++++++++++++++++++++++++++++++
>   util/meson.build         |  2 +-
>   2 files changed, 44 insertions(+), 1 deletion(-)
> 
> 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";

Thanks, queued wrapping the long lines to pass checkpatch.pl,
as in:
https://lore.kernel.org/qemu-devel/20251031211518.38503-9-philmd@linaro.org/