Re: [Qemu-devel] [PATCH for-2.12 7/8] tests: introduce qtest_init_with_qmp_format()

[Eric Blake]

On 03/26/2018 01:39 AM, Peter Xu wrote:
> It is abstracted from qtest_init_without_qmp_handshake(). It works just
> like qtest_init_without_qmp_handshake() but further it would allow the
> caller to specify the QMP parameter.
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>   tests/libqtest.c | 14 +++++++++++---
>   tests/libqtest.h | 14 ++++++++++++++
>   2 files changed, 25 insertions(+), 3 deletions(-)

[Reviewing in reverse order; you may want to look at 
scripts/git.orderfile for how to present your patches in a more logical 
manner with .h changes first.]

> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index 811169453a..1f3605ce73 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -62,6 +62,20 @@ QTestState *qtest_init(const char *extra_args);
>    */
>   QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>
> +/**
> + * qtest_init_with_qmp_format:
> + * @extra_args: other arguments to pass to QEMU.

Not your fault, but I'm already not a fan of 'extra_args'; it would be 
better to make these functions take an array of arguments, or even use 
varargs, instead of relying on shell word splitting.  Our testsuite is a 
gaping security hole if executed in a directory where a shell 
metacharacter causes the shell word splitting to do something different 
than planned.

> + * @qmp_format: format of QMP parameters, should contain one "%s"
> + *              field so that the socket path will be filled later.
> + *
> + * Note that this function will work just like
> + * qtest_init_without_qmp_handshake(), so no QMP handshake will be done.
> + *
> + * Returns: #QTestState instance.
> + */
> +QTestState *qtest_init_with_qmp_format(const char *extra_args,
> +                                       const char *qmp_format);

Ouch - you didn't use any attribute to mark the format string so that 
the compiler can enforce that it is treated as a printf-style argument. 
I wonder if it would have been better to just have a 'bool use_oob' 
parameter rather than playing ugly games with passing printf-style 
format arguments around.

> +
>   /**
>    * qtest_quit:
>    * @s: #QTestState instance to operate on.
>

> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 200b2b9e92..d2af1b17f0 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -166,19 +166,22 @@ static const char *qtest_qemu_binary(void)
>       return qemu_bin;
>   }
>   
> -QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> +QTestState *qtest_init_with_qmp_format(const char *extra_args,
> +                                       const char *qmp_format)
>   {
>       QTestState *s;
>       int sock, qmpsock, i;
>       gchar *socket_path;
>       gchar *qmp_socket_path;
>       gchar *command;
> +    gchar *qmp_params;
>       const char *qemu_binary = qtest_qemu_binary();
>   
>       s = g_new(QTestState, 1);
>   
>       socket_path = g_strdup_printf("/tmp/qtest-%d.sock", getpid());
>       qmp_socket_path = g_strdup_printf("/tmp/qtest-%d.qmp", getpid());
> +    qmp_params = g_strdup_printf(qmp_format, qmp_socket_path);

And in addition to my earlier comment about not using a compiler 
attribute, you aren't even bothering to assert that the caller didn't 
pass in a garbage string, which can really lead to weird breakages.

>   
>       /* It's possible that if an earlier test run crashed it might
>        * have left a stale unix socket lying around. Delete any
> @@ -199,12 +202,12 @@ QTestState *qtest_init_without_qmp_handshake(const char 
> *extra_args)
>           command = g_strdup_printf("exec %s "
>                                     "-qtest unix:%s,nowait "
>                                     "-qtest-log %s "
> -                                  "-qmp unix:%s,nowait "
> +                                  "%s "
>                                     "-machine accel=qtest "
>                                     "-display none "
>                                     "%s", qemu_binary, socket_path,
>                                     getenv("QTEST_LOG") ? "/dev/fd/2" : 
> "/dev/null",
> -                                  qmp_socket_path,
> +                                  qmp_params,

Again, if you used the idea of a 'bool use_oob',  this would look more like:

"-qmp unix:%s,nowait%s ",
...
qmp_socket_path, use_oob ? "x-oob=on" : "",

which is a lot more limited in scope to prevent auditing nightmares, 
while no less powerful for what you are actually using this new function 
for.

>                                     extra_args ?: "");
>           execlp("/bin/sh", "sh", "-c", command, NULL);
>           exit(1);
> @@ -237,6 +240,11 @@ QTestState *qtest_init_without_qmp_handshake(const char 
> *extra_args)
>       return s;
>   }
>   
> +QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> +{
> +    return qtest_init_with_qmp_format(extra_args, "-qmp unix:%s,nowait");
> +}

There are so few callers of qtest_init_without_qmp_handshake() that I'd 
just add the parameter to the existing function and update its two 
callers, instead of adding yet another forwarding wrapper.  Especially 
if it is just adding a 'bool use_oob' parameter.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org