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

[Greg Kurz <groug@kaod.org>]

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.

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