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

["Daniel P. Berrangé" <berrange@redhat.com>]

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 :|