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

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

[Greg Kurz]

The template pointer in virtio_9p_create_local_test_dir() is leaked.
Add the g_autofree annotation to fix that. While here, convert the
rest of the virtio 9p test code to using g_autofree or g_autoptr
where possible, since this is the preferred approach to avoid potential
leaks in the future.

Based-on: <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyte.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/qtest/libqos/virtio-9p.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

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);
     if (!local_test_path) {
         g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
     }
-    g_free(pwd);
 
     g_assert(local_test_path != NULL);
 
@@ -60,12 +59,11 @@ void virtio_9p_create_local_test_dir(void)
 void virtio_9p_remove_local_test_dir(void)
 {
     g_assert(local_test_path != NULL);
-    char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
+    g_autofree char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
     int res = system(cmd);
     if (res < 0) {
         /* ignore error, dummy check to prevent compiler error */
     }
-    g_free(cmd);
 }
 
 char *virtio_9p_test_path(const char *path)
@@ -209,8 +207,8 @@ static void *virtio_9p_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
 static void regex_replace(GString *haystack, const char *pattern,
                           const char *replace_fmt, ...)
 {
-    GRegex *regex;
-    char *replace, *s;
+    g_autoptr(GRegex) regex = NULL;
+    g_autofree char *replace = NULL, *s = NULL;
     va_list argp;
 
     va_start(argp, replace_fmt);
@@ -220,9 +218,6 @@ static void regex_replace(GString *haystack, const char *pattern,
     regex = g_regex_new(pattern, 0, 0, NULL);
     s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
     g_string_assign(haystack, s);
-    g_free(s);
-    g_regex_unref(regex);
-    g_free(replace);
 }
 
 void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)
-- 
2.34.1

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

[Thomas Huth]

On 26/01/2022 18.11, Greg Kurz wrote:
> The template pointer in virtio_9p_create_local_test_dir() is leaked.
> Add the g_autofree annotation to fix that. While here, convert the
> rest of the virtio 9p test code to using g_autofree or g_autoptr
> where possible, since this is the preferred approach to avoid potential
> leaks in the future.
> 
> Based-on: <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyte.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   tests/qtest/libqos/virtio-9p.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
> 
> 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);
>       if (!local_test_path) {
>           g_test_message("mkdtemp('%s') failed: %s", template, strerror(errno));
>       }
> -    g_free(pwd);
>   
>       g_assert(local_test_path != NULL);
>   
> @@ -60,12 +59,11 @@ void virtio_9p_create_local_test_dir(void)
>   void virtio_9p_remove_local_test_dir(void)
>   {
>       g_assert(local_test_path != NULL);
> -    char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
> +    g_autofree char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
>       int res = system(cmd);
>       if (res < 0) {
>           /* ignore error, dummy check to prevent compiler error */
>       }
> -    g_free(cmd);
>   }
>   
>   char *virtio_9p_test_path(const char *path)
> @@ -209,8 +207,8 @@ static void *virtio_9p_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
>   static void regex_replace(GString *haystack, const char *pattern,
>                             const char *replace_fmt, ...)
>   {
> -    GRegex *regex;
> -    char *replace, *s;
> +    g_autoptr(GRegex) regex = NULL;
> +    g_autofree char *replace = NULL, *s = NULL;
>       va_list argp;
>   
>       va_start(argp, replace_fmt);
> @@ -220,9 +218,6 @@ static void regex_replace(GString *haystack, const char *pattern,
>       regex = g_regex_new(pattern, 0, 0, NULL);
>       s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
>       g_string_assign(haystack, s);
> -    g_free(s);
> -    g_regex_unref(regex);
> -    g_free(replace);
>   }
>   
>   void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Christian Schoenebeck]

On Mittwoch, 26. Januar 2022 18:11:36 CET Greg Kurz wrote:
> The template pointer in virtio_9p_create_local_test_dir() is leaked.
> Add the g_autofree annotation to fix that. While here, convert the
> rest of the virtio 9p test code to using g_autofree or g_autoptr
> where possible, since this is the preferred approach to avoid potential
> leaks in the future.
> 
> Based-on:
> <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyte
> .com> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  tests/qtest/libqos/virtio-9p.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

I fear there is something wrong with this patch:

# Start of local tests
# starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest unix:/tmp/qtest-4234.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-4234.qmp,id=char0 -mon chardev=char0,mode=control -display none -M pc  -fsdev local,id=fsdev0,path='',security_model=mapped-xattr -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
qemu-system-x86_64: -device virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest: cannot initialize fsdev 'fsdev0': failed to open '': No such file or directory
Broken pipe
Aborted

> 
> 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);
>      if (!local_test_path) {
>          g_test_message("mkdtemp('%s') failed: %s", template,
> strerror(errno)); }
> -    g_free(pwd);
> 
>      g_assert(local_test_path != NULL);
> 
> @@ -60,12 +59,11 @@ void virtio_9p_create_local_test_dir(void)
>  void virtio_9p_remove_local_test_dir(void)
>  {
>      g_assert(local_test_path != NULL);
> -    char *cmd = g_strdup_printf("rm -fr '%s'\n", local_test_path);
> +    g_autofree char *cmd = g_strdup_printf("rm -fr '%s'\n",
> local_test_path); int res = system(cmd);
>      if (res < 0) {
>          /* ignore error, dummy check to prevent compiler error */
>      }
> -    g_free(cmd);
>  }
> 
>  char *virtio_9p_test_path(const char *path)
> @@ -209,8 +207,8 @@ static void *virtio_9p_pci_create(void *pci_bus,
> QGuestAllocator *t_alloc, static void regex_replace(GString *haystack,
> const char *pattern, const char *replace_fmt, ...)
>  {
> -    GRegex *regex;
> -    char *replace, *s;
> +    g_autoptr(GRegex) regex = NULL;
> +    g_autofree char *replace = NULL, *s = NULL;
>      va_list argp;
> 
>      va_start(argp, replace_fmt);
> @@ -220,9 +218,6 @@ static void regex_replace(GString *haystack, const char
> *pattern, regex = g_regex_new(pattern, 0, 0, NULL);
>      s = g_regex_replace(regex, haystack->str, -1, 0, replace, 0, NULL);
>      g_string_assign(haystack, s);
> -    g_free(s);
> -    g_regex_unref(regex);
> -    g_free(replace);
>  }
> 
>  void virtio_9p_assign_local_driver(GString *cmd_line, const char *args)

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

[Christian Schoenebeck]

On Freitag, 28. Januar 2022 12:49:58 CET Christian Schoenebeck wrote:
> On Mittwoch, 26. Januar 2022 18:11:36 CET Greg Kurz wrote:
> > The template pointer in virtio_9p_create_local_test_dir() is leaked.
> > Add the g_autofree annotation to fix that. While here, convert the
> > rest of the virtio 9p test code to using g_autofree or g_autoptr
> > where possible, since this is the preferred approach to avoid potential
> > leaks in the future.
> > 
> > Based-on:
> > <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyt
> > e
> > .com> Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> >  tests/qtest/libqos/virtio-9p.c | 15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> I fear there is something wrong with this patch:
> 
> # Start of local tests
> # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
> unix:/tmp/qtest-4234.sock -qtest-log /dev/null -chardev
> socket,path=/tmp/qtest-4234.qmp,id=char0 -mon chardev=char0,mode=control
> -display none -M pc  -fsdev
> local,id=fsdev0,path='',security_model=mapped-xattr -device
> virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> qemu-system-x86_64: -device
> virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest: cannot initialize
> fsdev 'fsdev0': failed to open '': No such file or directory Broken pipe
> Aborted

Reason ...

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

Best regards,
Christian Schoenebeck

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

[Greg Kurz]

On Sat, 29 Jan 2022 13:33:59 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 28. Januar 2022 12:49:58 CET Christian Schoenebeck wrote:
> > On Mittwoch, 26. Januar 2022 18:11:36 CET Greg Kurz wrote:
> > > The template pointer in virtio_9p_create_local_test_dir() is leaked.
> > > Add the g_autofree annotation to fix that. While here, convert the
> > > rest of the virtio 9p test code to using g_autofree or g_autoptr
> > > where possible, since this is the preferred approach to avoid potential
> > > leaks in the future.
> > > 
> > > Based-on:
> > > <f6602123c6f7d0d593466231b04fba087817abbd.1642879848.git.qemu_oss@crudebyt
> > > e
> > > .com> Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > > 
> > >  tests/qtest/libqos/virtio-9p.c | 15 +++++----------
> > >  1 file changed, 5 insertions(+), 10 deletions(-)
> > 
> > I fear there is something wrong with this patch:
> > 
> > # Start of local tests
> > # starting QEMU: exec x86_64-softmmu/qemu-system-x86_64 -qtest
> > unix:/tmp/qtest-4234.sock -qtest-log /dev/null -chardev
> > socket,path=/tmp/qtest-4234.qmp,id=char0 -mon chardev=char0,mode=control
> > -display none -M pc  -fsdev
> > local,id=fsdev0,path='',security_model=mapped-xattr -device
> > virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest -accel qtest
> > qemu-system-x86_64: -device
> > virtio-9p-pci,fsdev=fsdev0,addr=04.0,mount_tag=qtest: cannot initialize
> > fsdev 'fsdev0': failed to open '': No such file or directory Broken pipe
> > Aborted
> 
> Reason ...
> 
> > > 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.

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

> Best regards,
> Christian Schoenebeck
> 
> 

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

[Christian Schoenebeck]

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

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

[Greg Kurz]

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

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

[Christian Schoenebeck]

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

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

[Greg Kurz]

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

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

[Christian Schoenebeck]

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