Re: [Qemu-devel] [PATCH v7 28/38] libqtest: Add qtest_[v]startf()

[Thomas Huth]

On 11.09.2017 19:20, Eric Blake wrote:
> We have several callers that were formatting the argument strings
> themselves; consolidate this effort by adding new convenience
> functions directly in libqtest, and update all call-sites that
> can benefit from it.
[...]
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index e8c2e11817..b535d7768f 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -245,6 +245,27 @@ QTestState *qtest_start(const char *extra_args)
>      return global_qtest = s;
>  }
> 
> +QTestState *qtest_vstartf(const char *fmt, va_list ap)
> +{
> +    char *args = g_strdup_vprintf(fmt, ap);
> +    QTestState *s;
> +
> +    s = qtest_start(args);
> +    global_qtest = NULL;

Don't you need a g_free(args) here?

> +    return s;
> +}
> +
> +QTestState *qtest_startf(const char *fmt, ...)
> +{
> +    va_list ap;
> +    QTestState *s;
> +
> +    va_start(ap, fmt);
> +    s = qtest_vstartf(fmt, ap);
> +    va_end(ap);
> +    return s;
> +}
[...]
> diff --git a/tests/e1000-test.c b/tests/e1000-test.c
> index 0c5fcdcc44..12bc526ad6 100644
> --- a/tests/e1000-test.c
> +++ b/tests/e1000-test.c
> @@ -14,16 +14,8 @@
>  static void test_device(gconstpointer data)
>  {
>      const char *model = data;
> -    QTestState *s;
> -    char *args;
> 
> -    args = g_strdup_printf("-device %s", model);
> -    s = qtest_start(args);
> -
> -    if (s) {
> -        qtest_quit(s);
> -    }
> -    g_free(args);
> +    qtest_quit(qtest_startf("-device %s", model));

Just my personal taste, but I think I'd be nicer to keep this on
separate lines:

    QTestState *s;

    s = qtest_startf("-device %s", model);
    qtest_quit(s);

>  }
[...]
> diff --git a/tests/eepro100-test.c b/tests/eepro100-test.c
> index bdc8a67d57..fc9ea84d66 100644
> --- a/tests/eepro100-test.c
> +++ b/tests/eepro100-test.c
> @@ -13,18 +13,9 @@
>  static void test_device(gconstpointer data)
>  {
>      const char *model = data;
> -    QTestState *s;
> -    char *args;
> -
> -    args = g_strdup_printf("-device %s", model);
> -    s = qtest_start(args);
> 
>      /* Tests only initialization so far. TODO: Implement functional tests */
> -
> -    if (s) {
> -        qtest_quit(s);
> -    }
> -    g_free(args);
> +    qtest_quit(qtest_startf("-device %s", model));
>  }

dito

[...]
> diff --git a/tests/numa-test.c b/tests/numa-test.c
> index fa21d26935..e2f6c68be8 100644
> --- a/tests/numa-test.c
> +++ b/tests/numa-test.c
> @@ -12,20 +12,14 @@
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
> 
> -static char *make_cli(const char *generic_cli, const char *test_cli)
> -{
> -    return g_strdup_printf("%s %s", generic_cli ? generic_cli : "", 
> test_cli);
> -}
> -
>  static void test_mon_explicit(const void *data)
>  {
>      char *s;
> -    char *cli;
> +    const char *args = data;
> 
> -    cli = make_cli(data, "-smp 8 "
> -                   "-numa node,nodeid=0,cpus=0-3 "
> -                   "-numa node,nodeid=1,cpus=4-7 ");
> -    qtest_start(cli);
> +    global_qtest = qtest_startf("%s -smp 8 "
> +                                "-numa node,nodeid=0,cpus=0-3 "
> +                                "-numa node,nodeid=1,cpus=4-7 ", args);
> 
>      s = hmp("info numa");
>      g_assert(strstr(s, "node 0 cpus: 0 1 2 3"));
> @@ -33,16 +27,14 @@ static void test_mon_explicit(const void *data)
>      g_free(s);
> 
>      qtest_quit(global_qtest);
> -    g_free(cli);
>  }
> 
>  static void test_mon_default(const void *data)
>  {
>      char *s;
> -    char *cli;
> +    const char *args = data;
> 
> -    cli = make_cli(data, "-smp 8 -numa node -numa node");
> -    qtest_start(cli);
> +    global_qtest = qtest_startf("%s -smp 8 -numa node -numa node", args);
> 
>      s = hmp("info numa");
>      g_assert(strstr(s, "node 0 cpus: 0 2 4 6"));
> @@ -50,18 +42,16 @@ static void test_mon_default(const void *data)
>      g_free(s);
> 
>      qtest_quit(global_qtest);
> -    g_free(cli);
>  }
> 
>  static void test_mon_partial(const void *data)
>  {
>      char *s;
> -    char *cli;
> +    const char *args = data;
> 
> -    cli = make_cli(data, "-smp 8 "
> -                   "-numa node,nodeid=0,cpus=0-1 "
> -                   "-numa node,nodeid=1,cpus=4-5 ");
> -    qtest_start(cli);
> +    global_qtest = qtest_startf("%s -smp 8 "
> +                                "-numa node,nodeid=0,cpus=0-1 "
> +                                "-numa node,nodeid=1,cpus=4-5 ", args);

Does GCC emit a warning if you'd used data here directly? Otherwise I
think you could simply do this without the local args variable...

 Thomas