Re: [Qemu-devel] [PATCH 06/56] test-qga: Clean up how we test QGA synchronization

[Eric Blake]

On 08/08/2018 07:02 AM, Markus Armbruster wrote:
> To permit recovering from arbitrary JSON parse errors, the JSON parser
> resets itself on lexical errors.  We recommend sending a 0xff byte for
> that purpose, and test-qga covers this usage since commit 5229564b832.
> That commit had to add an ugly hack to qmp_fd_vsend() to make capable
> of sending this byte (it's designed to send only valid JSON).
>
> The previous commit added a way to send arbitrary text.  Put that to
> use for this purpose, and drop the hack from qmp_fd_vsend().
>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>   tests/libqtest.c | 39 +++++++++++++++++++++------------------
>   tests/libqtest.h |  2 ++
>   tests/test-qga.c |  8 ++++----
>   3 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index c02fc91b37..9c844874e4 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -489,16 +489,6 @@ void qmp_fd_vsend(int fd, const char *fmt, va_list ap)
>   {
>       QObject *qobj;
>   
> -    /*
> -     * qobject_from_vjsonf_nofail() chokes on leading 0xff as invalid
> -     * JSON, but tests/test-qga.c needs to send that to test QGA
> -     * synchronization
> -     */
> -    if (*fmt == '\377') {
> -        socket_send(fd, fmt, 1);
> -        fmt++;
> -    }
> -
>       /* Going through qobject ensures we escape strings properly */
>       qobj = qobject_from_vjsonf_nofail(fmt, ap);

This does JSON interpolation...

>   
> @@ -586,23 +576,36 @@ void qtest_qmp_send(QTestState *s, const char *fmt, ...)
>       va_end(ap);
>   }
>   
> -void qtest_qmp_send_raw(QTestState *s, const char *fmt, ...)
> +void qmp_fd_vsend_raw(int fd, const char *fmt, va_list ap)
>   {
>       bool log = getenv("QTEST_LOG") != NULL;
> -    va_list ap;
> -    char *str;
> -
> -    va_start(ap, fmt);
> -    str = g_strdup_vprintf(fmt, ap);
> -    va_end(ap);
> +    char *str = g_strdup_vprintf(fmt, ap);

...while the new code does printf interpolation...

> +++ b/tests/test-qga.c
> @@ -147,10 +147,10 @@ static void test_qga_sync_delimited(gconstpointer fix)
>       unsigned char c;
>       QDict *ret;
>   
> -    qmp_fd_send(fixture->fd,
> -                "\xff{'execute': 'guest-sync-delimited',"
> -                " 'arguments': {'id': %u } }",
> -                r);
> +    qmp_fd_send_raw(fixture->fd,
> +                    "\xff{'execute': 'guest-sync-delimited',"
> +                    " 'arguments': {'id': %u } }",
> +                    r);

...and your test was relying on interpolation. Fortunately, your only 
thing being interpolated (%u) happens to result in valid JSON - but you 
may want to split this into:

qmp_fd_send_raw(fixture->fd, "\xff");
qmp_fd_send(fixutre->fd, "{'execute ... %u}}", f);

to make it less questionable. If not, then call it out in the commit 
message and/or a comment on the code itself.

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