Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > --- > 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 <arm...@redhat.com>