[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 5/6] test-qga: Actually test 0xff sync bytes
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 5/6] test-qga: Actually test 0xff sync bytes |
Date: |
Thu, 19 Jan 2017 10:45:22 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> Commit 62c39b3 introduced test-qga, and at face value, appears
> to be testing the 'guest-sync' behavior that is recommended for
> guests in sending 0xff to QGA to force the parser to reset. But
> this aspect of the test has never actually done anything: the
> qmp_fd() call chain converts its string argument into QObject,
> then converts that QObject back to the actual string that is
> sent over the wire - and the conversion process silently drops
> the 0xff byte from the string sent to QGA,
This isn't immediately obvious from the code, but I assume you observed
it carefully.
> thus never resetting
> the QGA parser.
>
> An upcoming patch will get rid of the wasteful round trip
> through QObject, at which point the string in test-qga will be
> directly sent over the wire.
>
> But fixing qmp_fd() to actually send 0xff over the wire is not
> all we have to do - the actual QMP parser loudly complains that
> 0xff is not valid JSON, and sends an error message _prior_ to
> actually parsing the 'guest-sync' or 'guest-sync-delimited'
> command. With 'guest-sync', we cannot easily tell if this error
> message is a result of our command - which is WHY we invented
> the 'guest-sync-delimited' command. So for the testsuite, fix
> things to only check 0xff behavior on 'guest-sync-delimited',
> and to loop until we've consumed all garbage prior to the
> requested delimiter, which matches the documented actions that
> a real QGA client is supposed to do.
>
> Ideally, we'd fix the QGA JSON parser to silently ignore 0xff
> rather than sending an error message back, at which point we
> could enhance this test for 'guest-sync' as well as for
> 'guest-sync-delimited'. But for the sake of this patch, our
> testing of 'guest-sync' is no worse than it was pre-patch,
> because we have never been sending 0xff over the wire in the
> first place.
Is this worth a TODO comment, perhaps in test_qga_sync()?
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> tests/libqtest.c | 8 ++++++++
> tests/test-qga.c | 12 +++++++-----
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index d8fba66..3912e3e 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -430,6 +430,14 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
> va_list ap_copy;
> QObject *qobj;
>
> + /* qobject_from_jsonv() silently eats leading 0xff as invalid
> + * JSON, but we want to test sending them over the wire to force
> + * resyncs */
> + if (*fmt == '\377') {
> + socket_send(fd, fmt, 1);
> + fmt++;
> + }
> +
> /* Going through qobject ensures we escape strings properly.
> * This seemingly unnecessary copy is required in case va_list
> * is an array type.
> diff --git a/tests/test-qga.c b/tests/test-qga.c
> index 868b02a..4b64630 100644
> --- a/tests/test-qga.c
> +++ b/tests/test-qga.c
> @@ -151,9 +151,11 @@ static void test_qga_sync_delimited(gconstpointer fix)
cmd = g_strdup_printf("%c{'execute': 'guest-sync-delimited',"
" 'arguments': {'id': %u } }", 0xff, r);
Simplification opportunity:
cmd = g_strdup_printf("\xff{'execute': 'guest-sync-delimited',"
" 'arguments': {'id': %u } }", r);
> qmp_fd_send(fixture->fd, cmd);
> g_free(cmd);
>
> - v = read(fixture->fd, &c, 1);
> - g_assert_cmpint(v, ==, 1);
> - g_assert_cmpint(c, ==, 0xff);
> + /* Read and ignore garbage until resynchronized */
> + do {
> + v = read(fixture->fd, &c, 1);
> + g_assert_cmpint(v, ==, 1);
> + } while (c != 0xff);
Slow as molasses, but it'll do for this test.
>
> ret = qmp_fd_receive(fixture->fd);
> g_assert_nonnull(ret);
> @@ -172,8 +174,8 @@ static void test_qga_sync(gconstpointer fix)
> QDict *ret;
> gchar *cmd;
>
> - cmd = g_strdup_printf("%c{'execute': 'guest-sync',"
> - " 'arguments': {'id': %u } }", 0xff, r);
> + cmd = g_strdup_printf("{'execute': 'guest-sync',"
> + " 'arguments': {'id': %u } }", r);
> ret = qmp_fd(fixture->fd, cmd);
> g_free(cmd);
Reviewed-by: Markus Armbruster <address@hidden>
- Re: [Qemu-devel] [PATCH v2 1/6] pci: Use struct instead of QDict to pass back parameters, (continued)
- [Qemu-devel] [PATCH v2 6/6] qapi: Promote blockdev-change-medium arguments to QAPI type, Eric Blake, 2017/01/18
- [Qemu-devel] [PATCH v2 3/6] qlist: Add convenience helpers for wrapped appends, Eric Blake, 2017/01/18
- [Qemu-devel] [PATCH v2 5/6] test-qga: Actually test 0xff sync bytes, Eric Blake, 2017/01/18
- [Qemu-devel] [PATCH v2 4/6] fdc-test: Avoid deprecated 'change' command, Eric Blake, 2017/01/18
- [Qemu-devel] [PATCH v2 2/6] qdict: Add convenience helpers for wrapped puts, Eric Blake, 2017/01/18