| Message ID | 20170118161653.19296-6-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, Jan 18, 2017 at 8:39 PM Eric Blake <eblake@redhat.com> wrote: > 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, 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. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@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) > 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); > > 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); > > -- > 2.9.3 > > > -- Marc-André Lureau
Eric Blake <eblake@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 <eblake@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 <armbru@redhat.com>
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) 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); 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);
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, 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. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/libqtest.c | 8 ++++++++ tests/test-qga.c | 12 +++++++----- 2 files changed, 15 insertions(+), 5 deletions(-)