Re: [PATCH v2 83/86] tests:numa-test: make top level args dynamic and g_autofree(cli) cleanups

[Thomas Huth]

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 <address@hidden>
> ---
> PS:
>   made as a separate patch so it won't clutter followup testcase changes.
> 
> CC: address@hidden
> CC: address@hidden
> ---
>  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