[v2,83/86] tests:numa-test: make top level args dynamic and g_autofree(cli) cleanups
diff mbox series

Message ID 1579100861-73692-84-git-send-email-imammedo@redhat.com
State New
Headers show
Series
  • refactor main RAM allocation to use hostmem backend
Related show

Commit Message

Igor Mammedov Jan. 15, 2020, 3:07 p.m. UTC
Use GString to pass argument to make_cli() so that it would be easy
to dynamically change test case arguments from main(). The follow up
patch will use it to change RAM size options depending on target.

While at it cleanup 'cli' freeing, using g_autofree annotation.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
PS:
  made as a separate patch so it won't clutter followup testcase changes.

CC: thuth@redhat.com
CC: lvivier@redhat.com
---
 tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

Comments

Thomas Huth Jan. 16, 2020, 4:35 p.m. UTC | #1
On 15/01/2020 16.07, Igor Mammedov wrote:
> Use GString to pass argument to make_cli() so that it would be easy
> to dynamically change test case arguments from main(). The follow up
> patch will use it to change RAM size options depending on target.
> 
> While at it cleanup 'cli' freeing, using g_autofree annotation.

Hmm, I'd use g_autofree for new code or do it in a separate cleanup
patch, but doing this here distracts quite a bit from the real changes
that you are doing...

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> PS:
>   made as a separate patch so it won't clutter followup testcase changes.
> 
> CC: thuth@redhat.com
> CC: lvivier@redhat.com
> ---
>  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> index 17dd807..a696dfd 100644
> --- a/tests/qtest/numa-test.c
> +++ b/tests/qtest/numa-test.c
> @@ -14,16 +14,16 @@
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qlist.h"
>  
> -static char *make_cli(const char *generic_cli, const char *test_cli)
> +static char *make_cli(const GString *generic_cli, const char *test_cli)
>  {
> -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
> +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
>  }
[...]
> @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
>  
>  int main(int argc, char **argv)
>  {
> -    const char *args = NULL;
> +    g_autoptr(GString) args = g_string_new("");

I think g_string_new(NULL) would be better?

>      const char *arch = qtest_get_arch();
>  
>      if (strcmp(arch, "aarch64") == 0) {
> -        args = "-machine virt";
> +        g_string_append(args, " -machine virt")>      }

Is this really required? Looking at your next patch, you could also
simply do

          args = " -object memory-backend-ram,id=ram,size=xxxM"

there? So using a GString seems overkill to me here.

 Thomas
Igor Mammedov Jan. 16, 2020, 5:06 p.m. UTC | #2
On Thu, 16 Jan 2020 17:35:32 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 15/01/2020 16.07, Igor Mammedov wrote:
> > Use GString to pass argument to make_cli() so that it would be easy
> > to dynamically change test case arguments from main(). The follow up
> > patch will use it to change RAM size options depending on target.
> > 
> > While at it cleanup 'cli' freeing, using g_autofree annotation.  
> 
> Hmm, I'd use g_autofree for new code or do it in a separate cleanup
> patch, but doing this here distracts quite a bit from the real changes
> that you are doing...
I'll split it into separate patch

> 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > PS:
> >   made as a separate patch so it won't clutter followup testcase changes.
> > 
> > CC: thuth@redhat.com
> > CC: lvivier@redhat.com
> > ---
> >  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
> >  1 file changed, 14 insertions(+), 24 deletions(-)
> > 
> > diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> > index 17dd807..a696dfd 100644
> > --- a/tests/qtest/numa-test.c
> > +++ b/tests/qtest/numa-test.c
> > @@ -14,16 +14,16 @@
> >  #include "qapi/qmp/qdict.h"
> >  #include "qapi/qmp/qlist.h"
> >  
> > -static char *make_cli(const char *generic_cli, const char *test_cli)
> > +static char *make_cli(const GString *generic_cli, const char *test_cli)
> >  {
> > -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
> > +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
> >  }  
> [...]
> > @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
> >  
> >  int main(int argc, char **argv)
> >  {
> > -    const char *args = NULL;
> > +    g_autoptr(GString) args = g_string_new("");  
> 
> I think g_string_new(NULL) would be better?
> 
> >      const char *arch = qtest_get_arch();
> >  
> >      if (strcmp(arch, "aarch64") == 0) {
> > -        args = "-machine virt";
> > +        g_string_append(args, " -machine virt")>      }  
> 
> Is this really required? Looking at your next patch, you could also
> simply do
> 
>           args = " -object memory-backend-ram,id=ram,size=xxxM"
xxx is variable so options are
 1 build this part of CLI dynamically
 2 mostly duplicate testcase function and include per target size there
 3 make up a test data structure and pass that to test cases

Given simplicity of current testcases, I'd prefer continue with
passing CLI as testcase data (option #1).


> 
> there? So using a GString seems overkill to me here.
> 
>  Thomas
Thomas Huth Jan. 17, 2020, 11:14 a.m. UTC | #3
On 16/01/2020 18.06, Igor Mammedov wrote:
> On Thu, 16 Jan 2020 17:35:32 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 15/01/2020 16.07, Igor Mammedov wrote:
>>> Use GString to pass argument to make_cli() so that it would be easy
>>> to dynamically change test case arguments from main(). The follow up
>>> patch will use it to change RAM size options depending on target.
>>>
>>> While at it cleanup 'cli' freeing, using g_autofree annotation.  
>>
>> Hmm, I'd use g_autofree for new code or do it in a separate cleanup
>> patch, but doing this here distracts quite a bit from the real changes
>> that you are doing...
> I'll split it into separate patch
> 
>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> PS:
>>>   made as a separate patch so it won't clutter followup testcase changes.
>>>
>>> CC: thuth@redhat.com
>>> CC: lvivier@redhat.com
>>> ---
>>>  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
>>>  1 file changed, 14 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
>>> index 17dd807..a696dfd 100644
>>> --- a/tests/qtest/numa-test.c
>>> +++ b/tests/qtest/numa-test.c
>>> @@ -14,16 +14,16 @@
>>>  #include "qapi/qmp/qdict.h"
>>>  #include "qapi/qmp/qlist.h"
>>>  
>>> -static char *make_cli(const char *generic_cli, const char *test_cli)
>>> +static char *make_cli(const GString *generic_cli, const char *test_cli)
>>>  {
>>> -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
>>> +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
>>>  }  
>> [...]
>>> @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
>>>  
>>>  int main(int argc, char **argv)
>>>  {
>>> -    const char *args = NULL;
>>> +    g_autoptr(GString) args = g_string_new("");  
>>
>> I think g_string_new(NULL) would be better?
>>
>>>      const char *arch = qtest_get_arch();
>>>  
>>>      if (strcmp(arch, "aarch64") == 0) {
>>> -        args = "-machine virt";
>>> +        g_string_append(args, " -machine virt")>      }  
>>
>> Is this really required? Looking at your next patch, you could also
>> simply do
>>
>>           args = " -object memory-backend-ram,id=ram,size=xxxM"
> xxx is variable so options are
>  1 build this part of CLI dynamically
>  2 mostly duplicate testcase function and include per target size there
>  3 make up a test data structure and pass that to test cases
> 
> Given simplicity of current testcases, I'd prefer continue with
> passing CLI as testcase data (option #1).

Sorry, I think I missed something here... currently I see in the next patch:

+    if (!strcmp(arch, "ppc64")) {
+        g_string_append(args, " -object
memory-backend-ram,id=ram,size=512M");
+    } else {
+        g_string_append(args, " -object
memory-backend-ram,id=ram,size=128M");
+    }

... so these are static strings which could also be handled fine without
GString? Or do you plan to update this in later patches?

 Thomas
Igor Mammedov Jan. 17, 2020, 1:33 p.m. UTC | #4
On Fri, 17 Jan 2020 12:14:11 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 16/01/2020 18.06, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 17:35:32 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 15/01/2020 16.07, Igor Mammedov wrote:  
> >>> Use GString to pass argument to make_cli() so that it would be easy
> >>> to dynamically change test case arguments from main(). The follow up
> >>> patch will use it to change RAM size options depending on target.
> >>>
> >>> While at it cleanup 'cli' freeing, using g_autofree annotation.    
> >>
> >> Hmm, I'd use g_autofree for new code or do it in a separate cleanup
> >> patch, but doing this here distracts quite a bit from the real changes
> >> that you are doing...  
> > I'll split it into separate patch
> >   
> >>  
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>> PS:
> >>>   made as a separate patch so it won't clutter followup testcase changes.
> >>>
> >>> CC: thuth@redhat.com
> >>> CC: lvivier@redhat.com
> >>> ---
> >>>  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
> >>>  1 file changed, 14 insertions(+), 24 deletions(-)
> >>>
> >>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> >>> index 17dd807..a696dfd 100644
> >>> --- a/tests/qtest/numa-test.c
> >>> +++ b/tests/qtest/numa-test.c
> >>> @@ -14,16 +14,16 @@
> >>>  #include "qapi/qmp/qdict.h"
> >>>  #include "qapi/qmp/qlist.h"
> >>>  
> >>> -static char *make_cli(const char *generic_cli, const char *test_cli)
> >>> +static char *make_cli(const GString *generic_cli, const char *test_cli)
> >>>  {
> >>> -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
> >>> +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
> >>>  }    
> >> [...]  
> >>> @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
> >>>  
> >>>  int main(int argc, char **argv)
> >>>  {
> >>> -    const char *args = NULL;
> >>> +    g_autoptr(GString) args = g_string_new("");    
> >>
> >> I think g_string_new(NULL) would be better?
> >>  
> >>>      const char *arch = qtest_get_arch();
> >>>  
> >>>      if (strcmp(arch, "aarch64") == 0) {
> >>> -        args = "-machine virt";
> >>> +        g_string_append(args, " -machine virt")>      }    
> >>
> >> Is this really required? Looking at your next patch, you could also
> >> simply do
> >>
> >>           args = " -object memory-backend-ram,id=ram,size=xxxM"  
> > xxx is variable so options are
> >  1 build this part of CLI dynamically
> >  2 mostly duplicate testcase function and include per target size there
> >  3 make up a test data structure and pass that to test cases
> > 
> > Given simplicity of current testcases, I'd prefer continue with
> > passing CLI as testcase data (option #1).  
> 
> Sorry, I think I missed something here... currently I see in the next patch:
> 
> +    if (!strcmp(arch, "ppc64")) {
> +        g_string_append(args, " -object
> memory-backend-ram,id=ram,size=512M");
> +    } else {
> +        g_string_append(args, " -object
> memory-backend-ram,id=ram,size=128M");
> +    }
> 
> ... so these are static strings which could also be handled fine without
> GString? Or do you plan to update this in later patches?
it's 1 or concatenation of 2 strings
  " -object memory-backend-ram,id=ram,size=128M"
  " -object memory-backend-ram,id=ram,size=512M"
  " -machine virt" + " -object memory-backend-ram,id=ram,size=128M"

It's possible to use static strings in expense of a bit of duplication
and long winded "if" chain. So using dynamic string here, looks better to me.

>  Thomas
Thomas Huth Jan. 17, 2020, 1:52 p.m. UTC | #5
On 17/01/2020 14.33, Igor Mammedov wrote:
> On Fri, 17 Jan 2020 12:14:11 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 16/01/2020 18.06, Igor Mammedov wrote:
>>> On Thu, 16 Jan 2020 17:35:32 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>   
>>>> On 15/01/2020 16.07, Igor Mammedov wrote:  
>>>>> Use GString to pass argument to make_cli() so that it would be easy
>>>>> to dynamically change test case arguments from main(). The follow up
>>>>> patch will use it to change RAM size options depending on target.
>>>>>
>>>>> While at it cleanup 'cli' freeing, using g_autofree annotation.    
>>>>
>>>> Hmm, I'd use g_autofree for new code or do it in a separate cleanup
>>>> patch, but doing this here distracts quite a bit from the real changes
>>>> that you are doing...  
>>> I'll split it into separate patch
>>>   
>>>>  
>>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>>> ---
>>>>> PS:
>>>>>   made as a separate patch so it won't clutter followup testcase changes.
>>>>>
>>>>> CC: thuth@redhat.com
>>>>> CC: lvivier@redhat.com
>>>>> ---
>>>>>  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
>>>>>  1 file changed, 14 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
>>>>> index 17dd807..a696dfd 100644
>>>>> --- a/tests/qtest/numa-test.c
>>>>> +++ b/tests/qtest/numa-test.c
>>>>> @@ -14,16 +14,16 @@
>>>>>  #include "qapi/qmp/qdict.h"
>>>>>  #include "qapi/qmp/qlist.h"
>>>>>  
>>>>> -static char *make_cli(const char *generic_cli, const char *test_cli)
>>>>> +static char *make_cli(const GString *generic_cli, const char *test_cli)
>>>>>  {
>>>>> -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
>>>>> +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
>>>>>  }    
>>>> [...]  
>>>>> @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
>>>>>  
>>>>>  int main(int argc, char **argv)
>>>>>  {
>>>>> -    const char *args = NULL;
>>>>> +    g_autoptr(GString) args = g_string_new("");    
>>>>
>>>> I think g_string_new(NULL) would be better?
>>>>  
>>>>>      const char *arch = qtest_get_arch();
>>>>>  
>>>>>      if (strcmp(arch, "aarch64") == 0) {
>>>>> -        args = "-machine virt";
>>>>> +        g_string_append(args, " -machine virt")>      }    
>>>>
>>>> Is this really required? Looking at your next patch, you could also
>>>> simply do
>>>>
>>>>           args = " -object memory-backend-ram,id=ram,size=xxxM"  
>>> xxx is variable so options are
>>>  1 build this part of CLI dynamically
>>>  2 mostly duplicate testcase function and include per target size there
>>>  3 make up a test data structure and pass that to test cases
>>>
>>> Given simplicity of current testcases, I'd prefer continue with
>>> passing CLI as testcase data (option #1).  
>>
>> Sorry, I think I missed something here... currently I see in the next patch:
>>
>> +    if (!strcmp(arch, "ppc64")) {
>> +        g_string_append(args, " -object
>> memory-backend-ram,id=ram,size=512M");
>> +    } else {
>> +        g_string_append(args, " -object
>> memory-backend-ram,id=ram,size=128M");
>> +    }
>>
>> ... so these are static strings which could also be handled fine without
>> GString? Or do you plan to update this in later patches?
> it's 1 or concatenation of 2 strings
>   " -object memory-backend-ram,id=ram,size=128M"
>   " -object memory-backend-ram,id=ram,size=512M"
>   " -machine virt" + " -object memory-backend-ram,id=ram,size=128M"

Ah, well, that's what I was missing. Since the if-arch-statements follow
close to each other, I somehow read 'else if (!strcmp(arch, "ppc64"))'
... sorry for that.
Maybe it's more obvious if you'd do it the other way round, first the
"-object" lines, then the "-machine virt" stuff?

Anyway, another note: It's a little bit ugly that one if-statment uses
strcmp() != 0 while the other one uses !strcmp() ... since you're using
GLIB code here anyway, what do you think about converting them to
g_str_equal() instead?

 Thomas
Igor Mammedov Jan. 17, 2020, 2:02 p.m. UTC | #6
On Fri, 17 Jan 2020 14:52:35 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 17/01/2020 14.33, Igor Mammedov wrote:
> > On Fri, 17 Jan 2020 12:14:11 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 16/01/2020 18.06, Igor Mammedov wrote:  
> >>> On Thu, 16 Jan 2020 17:35:32 +0100
> >>> Thomas Huth <thuth@redhat.com> wrote:
> >>>     
> >>>> On 15/01/2020 16.07, Igor Mammedov wrote:    
> >>>>> Use GString to pass argument to make_cli() so that it would be easy
> >>>>> to dynamically change test case arguments from main(). The follow up
> >>>>> patch will use it to change RAM size options depending on target.
> >>>>>
> >>>>> While at it cleanup 'cli' freeing, using g_autofree annotation.      
> >>>>
> >>>> Hmm, I'd use g_autofree for new code or do it in a separate cleanup
> >>>> patch, but doing this here distracts quite a bit from the real changes
> >>>> that you are doing...    
> >>> I'll split it into separate patch
> >>>     
> >>>>    
> >>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>>> ---
> >>>>> PS:
> >>>>>   made as a separate patch so it won't clutter followup testcase changes.
> >>>>>
> >>>>> CC: thuth@redhat.com
> >>>>> CC: lvivier@redhat.com
> >>>>> ---
> >>>>>  tests/qtest/numa-test.c | 38 ++++++++++++++------------------------
> >>>>>  1 file changed, 14 insertions(+), 24 deletions(-)
> >>>>>
> >>>>> diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
> >>>>> index 17dd807..a696dfd 100644
> >>>>> --- a/tests/qtest/numa-test.c
> >>>>> +++ b/tests/qtest/numa-test.c
> >>>>> @@ -14,16 +14,16 @@
> >>>>>  #include "qapi/qmp/qdict.h"
> >>>>>  #include "qapi/qmp/qlist.h"
> >>>>>  
> >>>>> -static char *make_cli(const char *generic_cli, const char *test_cli)
> >>>>> +static char *make_cli(const GString *generic_cli, const char *test_cli)
> >>>>>  {
> >>>>> -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
> >>>>> +    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
> >>>>>  }      
> >>>> [...]    
> >>>>> @@ -539,11 +529,11 @@ static void pc_hmat_erange_cfg(const void *data)
> >>>>>  
> >>>>>  int main(int argc, char **argv)
> >>>>>  {
> >>>>> -    const char *args = NULL;
> >>>>> +    g_autoptr(GString) args = g_string_new("");      
> >>>>
> >>>> I think g_string_new(NULL) would be better?
> >>>>    
> >>>>>      const char *arch = qtest_get_arch();
> >>>>>  
> >>>>>      if (strcmp(arch, "aarch64") == 0) {
> >>>>> -        args = "-machine virt";
> >>>>> +        g_string_append(args, " -machine virt")>      }      
> >>>>
> >>>> Is this really required? Looking at your next patch, you could also
> >>>> simply do
> >>>>
> >>>>           args = " -object memory-backend-ram,id=ram,size=xxxM"    
> >>> xxx is variable so options are
> >>>  1 build this part of CLI dynamically
> >>>  2 mostly duplicate testcase function and include per target size there
> >>>  3 make up a test data structure and pass that to test cases
> >>>
> >>> Given simplicity of current testcases, I'd prefer continue with
> >>> passing CLI as testcase data (option #1).    
> >>
> >> Sorry, I think I missed something here... currently I see in the next patch:
> >>
> >> +    if (!strcmp(arch, "ppc64")) {
> >> +        g_string_append(args, " -object
> >> memory-backend-ram,id=ram,size=512M");
> >> +    } else {
> >> +        g_string_append(args, " -object
> >> memory-backend-ram,id=ram,size=128M");
> >> +    }
> >>
> >> ... so these are static strings which could also be handled fine without
> >> GString? Or do you plan to update this in later patches?  
> > it's 1 or concatenation of 2 strings
> >   " -object memory-backend-ram,id=ram,size=128M"
> >   " -object memory-backend-ram,id=ram,size=512M"
> >   " -machine virt" + " -object memory-backend-ram,id=ram,size=128M"  
> 
> Ah, well, that's what I was missing. Since the if-arch-statements follow
> close to each other, I somehow read 'else if (!strcmp(arch, "ppc64"))'
> ... sorry for that.
> Maybe it's more obvious if you'd do it the other way round, first the
> "-object" lines, then the "-machine virt" stuff?
> 
> Anyway, another note: It's a little bit ugly that one if-statment uses
> strcmp() != 0 while the other one uses !strcmp() ... since you're using
> GLIB code here anyway, what do you think about converting them to
> g_str_equal() instead?

will do that

> 
>  Thomas

Patch
diff mbox series

diff --git a/tests/qtest/numa-test.c b/tests/qtest/numa-test.c
index 17dd807..a696dfd 100644
--- a/tests/qtest/numa-test.c
+++ b/tests/qtest/numa-test.c
@@ -14,16 +14,16 @@ 
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qlist.h"
 
-static char *make_cli(const char *generic_cli, const char *test_cli)
+static char *make_cli(const GString *generic_cli, const char *test_cli)
 {
-    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", test_cli);
+    return g_strdup_printf("%s %s", generic_cli->str, test_cli);
 }
 
 static void test_mon_explicit(const void *data)
 {
-    char *s;
-    char *cli;
     QTestState *qts;
+    g_autofree char *s = NULL;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-smp 8 "
                    "-numa node,nodeid=0,cpus=0-3 "
@@ -33,17 +33,15 @@  static void test_mon_explicit(const void *data)
     s = qtest_hmp(qts, "info numa");
     g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
     g_assert(strstr(s, "node 1 cpus: 4 5 6 7"));
-    g_free(s);
 
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static void test_mon_default(const void *data)
 {
-    char *s;
-    char *cli;
     QTestState *qts;
+    g_autofree char *s = NULL;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-smp 8 -numa node -numa node");
     qts = qtest_init(cli);
@@ -51,17 +49,15 @@  static void test_mon_default(const void *data)
     s = qtest_hmp(qts, "info numa");
     g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
     g_assert(strstr(s, "node 1 cpus: 1 3 5 7"));
-    g_free(s);
 
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static void test_mon_partial(const void *data)
 {
-    char *s;
-    char *cli;
     QTestState *qts;
+    g_autofree char *s = NULL;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-smp 8 "
                    "-numa node,nodeid=0,cpus=0-1 "
@@ -71,10 +67,8 @@  static void test_mon_partial(const void *data)
     s = qtest_hmp(qts, "info numa");
     g_assert(strstr(s, "node 0 cpus: 0 1 2 3 6 7"));
     g_assert(strstr(s, "node 1 cpus: 4 5"));
-    g_free(s);
 
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static QList *get_cpus(QTestState *qts, QDict **resp)
@@ -87,11 +81,11 @@  static QList *get_cpus(QTestState *qts, QDict **resp)
 
 static void test_query_cpus(const void *data)
 {
-    char *cli;
     QDict *resp;
     QList *cpus;
     QObject *e;
     QTestState *qts;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-smp 8 -numa node,cpus=0-3 -numa node,cpus=4-7");
     qts = qtest_init(cli);
@@ -120,16 +114,15 @@  static void test_query_cpus(const void *data)
 
     qobject_unref(resp);
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static void pc_numa_cpu(const void *data)
 {
-    char *cli;
     QDict *resp;
     QList *cpus;
     QObject *e;
     QTestState *qts;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-cpu pentium -smp 8,sockets=2,cores=2,threads=2 "
         "-numa node,nodeid=0 -numa node,nodeid=1 "
@@ -174,16 +167,15 @@  static void pc_numa_cpu(const void *data)
 
     qobject_unref(resp);
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static void spapr_numa_cpu(const void *data)
 {
-    char *cli;
     QDict *resp;
     QList *cpus;
     QObject *e;
     QTestState *qts;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-smp 4,cores=4 "
         "-numa node,nodeid=0 -numa node,nodeid=1 "
@@ -220,16 +212,15 @@  static void spapr_numa_cpu(const void *data)
 
     qobject_unref(resp);
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static void aarch64_numa_cpu(const void *data)
 {
-    char *cli;
     QDict *resp;
     QList *cpus;
     QObject *e;
     QTestState *qts;
+    g_autofree char *cli = NULL;
 
     cli = make_cli(data, "-smp 2 "
         "-numa node,nodeid=0 -numa node,nodeid=1 "
@@ -264,7 +255,6 @@  static void aarch64_numa_cpu(const void *data)
 
     qobject_unref(resp);
     qtest_quit(qts);
-    g_free(cli);
 }
 
 static void pc_dynamic_cpu_cfg(const void *data)
@@ -539,11 +529,11 @@  static void pc_hmat_erange_cfg(const void *data)
 
 int main(int argc, char **argv)
 {
-    const char *args = NULL;
+    g_autoptr(GString) args = g_string_new("");
     const char *arch = qtest_get_arch();
 
     if (strcmp(arch, "aarch64") == 0) {
-        args = "-machine virt";
+        g_string_append(args, " -machine virt");
     }
 
     g_test_init(&argc, &argv, NULL);