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

[Greg Kurz <groug@kaod.org>]

On Mon, 31 Jan 2022 13:37:23 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

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

An alternative might be to create/remove the test directory when
a virtio-9p device is started/destroyed, and keeping the string
under the QVirtio9p structure. The most notable effect would be
to have a new directory for each individual test instead of
all the lifetime of qos-test, but it doesn't hurt. I'll have a look.

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

Sure, maybe misleading isn't the right wording, but it certainly
tricked my into adding g_autofree while completely missing the
pointer ended up in a global... :-)

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