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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Montag, 31. Januar 2022 17:09:07 CET Greg Kurz wrote:
> On Mon, 31 Jan 2022 16:12:45 +0100
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Montag, 31. Januar 2022 15:44:46 CET Greg Kurz wrote:
> > > 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.
> > 
> > Yeah, I tried that already. Keep in mind it not only has to work
> > sometimes, it has to work reliably, always, for everybody and commit
> > history shows that this can be more hairy than one might think and
> > observe.
> 
> Yeah it is more hairy... the temp directory must be created before the
> device. We could maybe get rid of the constructor by creating the temp
> direcotry in assign_9p_local_driver() since this is the first user. Then we
> still need the destructor to do final cleanup.

I save your time on that: it doesn't work. I tried that as well, plus probably 
a bunch of other options that you haven't considered yet. I even reviewed the 
entire libqos and glib test case code base to find a clean alternative, 
without success.

Best regards,
Christian Schoenebeck