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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

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.

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

I'd like to avoid just converting one compromise into another one.

If I had to choose between fixing a purely theoretical issue of getting rid of 
a global variable, or introducing a real-life issue in form of numerous new 
test dirs popping up on toplevel, I rather stick to the former. We already 
have enough test dirs popping up on toplevel IMO.

A truly clean solution for this would be the introduction of setup/teardown 
callback pairs in libqos, like it is standard in other test suites. No plans 
on my side for spending coding time on that in near future though. My review 
time for patches on that being assured though.

Best regards,
Christian Schoenebeck