Re: [PATCH] tests/9pfs: Use g_autofree and g_autoptr where possible

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 31. Januar 2022 08:35:24 CET Greg Kurz wrote:
> > > > diff --git a/tests/qtest/libqos/virtio-9p.c
> > > > b/tests/qtest/libqos/virtio-9p.c index ef96ef006adc..0a0d0d16709b
> > > > 100644
> > > > --- a/tests/qtest/libqos/virtio-9p.c
> > > > +++ b/tests/qtest/libqos/virtio-9p.c
> > > > @@ -40,14 +40,13 @@ static char *concat_path(const char* a, const
> > > > char* b)
> > > > 
> > > >  void virtio_9p_create_local_test_dir(void)
> > > >  {
> > > >  
> > > >      struct stat st;
> > > > 
> > > > -    char *pwd = g_get_current_dir();
> > > > -    char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
> > > > +    g_autofree char *pwd = g_get_current_dir();
> > > > +    g_autofree char *template = concat_path(pwd,
> > > > "qtest-9p-local-XXXXXX");
> > > > 
> > > >      local_test_path = mkdtemp(template);
> > 
> > ... mkdtemp() does not allocate a new buffer, it just modifies the
> > character array passed, i.e. the address returned by mkdtemp() equals the
> > address of variable 'template', and when
> > virtio_9p_create_local_test_dir() scope is left, the global variable
> > 'local_test_path' would then point to freed memory.
> I hate global variables ;-) and the 'Returned result must be freed' comment
> in 'concat_path()' is slightly misleading in this respect.

About the global variable: sure, I am not happy about it either. What I
disliked even more is that virtio_9p_create_local_test_dir() is called from a
constructor, but as I described in [1] I did not find a realiable alternative.
If somebody comes up with a working and reliable, clean alternative, very much
appreciated!

About the concat_path() comment: I don't understand what's supposed to be
misleading about the comment, concat_path() is just a one-liner utility
function:

/* Concatenates the passed 2 pathes. Returned result must be freed. */
static char *concat_path(const char* a, const char* b)
{
    return g_build_filename(a, b, NULL);
}

So all the comment sais is that the function allocates memory that it does not
free on it its own. The called glib function sais this [2]:

    "A newly-allocated string that must be freed with g_free()."

[1] https://github.com/qemu/qemu/commit/136b7af22774a6f0fb44c9c1b8c088b52e2e92ed
[2] https://docs.gtk.org/glib/func.build_filename.html

> 
> > I would drop g_autofree from template:
> >     char *template = concat_path(pwd, "qtest-9p-local-XXXXXX");
> > 
> > And if it helps to silence a leak warning (haven't tested), to prepend
> > g_autofree to the global variable instead:
> > 
> > static g_autofree char *local_test_path;
> 
> The way to go is either drop the g_autofree annotation as you're
> suggesting, but this would make the comment in 'concat_path()'
> even more awkward, or go forward with the glib way and use
> g_steal_pointer() which maps exactly to what the code is doing.

I am fine either way, as long as the resulting behaviour works.

Best regards,
Christian Schoenebeck