Re: [Qemu-devel] [PATCH v6 27/29] libqtest: Make qtest_init() accept format string

[Thomas Huth]

On 01.09.2017 20:03, Eric Blake wrote:
> Several callers were formatting a string into a temporary
> variable before calling qtest_init(); factor that into the
> common code.
> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  tests/libqtest.h         |  7 ++++---
>  tests/libqtest.c         | 13 ++++++++++--
>  tests/bios-tables-test.c |  7 +------
>  tests/postcopy-test.c    |  4 ++--
>  tests/vhost-user-test.c  | 51 
> ++++++++++++++----------------------------------
>  5 files changed, 33 insertions(+), 49 deletions(-)
> 
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 3ae570927a..d338de3e22 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -25,11 +25,12 @@ extern QTestState *global_qtest;
> 
>  /**
>   * qtest_init:
> - * @extra_args: other arguments to pass to QEMU.
> + * @extra_args...: other arguments to pass to QEMU, formatted as if by
> + * sprintf().
>   *
>   * Returns: #QTestState instance.
>   */
> -QTestState *qtest_init(const char *extra_args);
> +QTestState *qtest_init(const char *extra_args, ...) GCC_FMT_ATTR(1, 2);

Maybe the parameter should be called "fmt" or "format" now instead?
(similar to printf() and friends)

>  /**
>   * qtest_init_without_qmp_handshake:
> @@ -518,7 +519,7 @@ void qtest_add_abrt_handler(GHookFunc fn, const void 
> *data);
>   */
>  static inline QTestState *qtest_start(const char *args)
>  {
> -    global_qtest = qtest_init(args);
> +    global_qtest = qtest_init("%s", args);

That's a little bit sad that there are some spots which need an explicit
"%s" now ... maybe it would be nicer to add a new function called
qtest_initf() instead and keep the old one (without the "f") as it is?

>      return global_qtest;
>  }
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index b6dd26e54a..18facbf130 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -347,9 +347,18 @@ QTestState *qtest_init_without_qmp_handshake(const char 
> *extra_args)
>      return s;
>  }
> 
> -QTestState *qtest_init(const char *extra_args)
> +QTestState *qtest_init(const char *extra_args, ...)
>  {
> -    QTestState *s = qtest_init_without_qmp_handshake(extra_args);
> +    va_list ap;
> +    QTestState *s;
> +    char *cmd;
> +
> +    va_start(ap, extra_args);
> +    cmd = g_strdup_vprintf(extra_args, ap);
> +    va_end(ap);
> +
> +    s = qtest_init_without_qmp_handshake(cmd);
> +    g_free(cmd);
> 
>      /* Read the QMP greeting and then do the handshake */
>      qtest_qmp_discard_response(s, "");
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 976792f2c5..f8a107499f 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -623,18 +623,14 @@ static void test_smbios_structs(test_data *data)
> 
>  static void test_acpi_one(const char *params, test_data *data)
>  {
> -    char *args;
> -
>      /* Disable kernel irqchip to be able to override apic irq0. */
> -    args = g_strdup_printf("-machine %s,accel=%s,kernel-irqchip=off "
> +    data->qts = qtest_init("-machine %s,accel=%s,kernel-irqchip=off "
>                             "-net none -display none %s "
>                             "-drive id=hd0,if=none,file=%s,format=raw "
>                             "-device ide-hd,drive=hd0 ",
>                             data->machine, "kvm:tcg",
>                             params ? params : "", disk);
> 
> -    data->qts = qtest_init(args);
> -
>      boot_sector_test(data->qts);
> 
>      test_acpi_rsdp_address(data);
> @@ -657,7 +653,6 @@ static void test_acpi_one(const char *params, test_data 
> *data)
>      test_smbios_structs(data);
> 
>      qtest_quit(data->qts);
> -    g_free(args);
>  }
> 
>  static uint8_t base_required_struct_types[] = {
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index 0e5fe20a83..20cfb280f6 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -403,10 +403,10 @@ static void test_migrate(void)
> 
>      g_free(bootpath);
> 
> -    from = qtest_init(cmd_src);
> +    from = qtest_init("%s", cmd_src);
>      g_free(cmd_src);
> 
> -    to = qtest_init(cmd_dst);
> +    to = qtest_init("%s", cmd_dst);
>      g_free(cmd_dst);
> 
>      assert(!global_qtest);

 Thomas