| Message ID | 20170427215821.19397-11-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Eric Blake (2017-04-27 16:58:21) > 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. The full re-sync sequence is actually to perform that process, check if the response matches the client-provided sync token, and then repeat the procedure if it doesn't (in the odd case that a previous client initiated a guest-sync-delimited sequence and never consumed the response). The current implementation only performs one iteration so it's not quite a full implementation of the documentation procedure. For the immediate purpose of improving the handling to deal with the 0xFF-generated error the patch seems sound though, maybe just something worth noting in the commit msg or comments so that we might eventually test the full procedure. In any case: Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > 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: Markus Armbruster <armbru@redhat.com> > > --- > v5: add R-b > v4: no change > v3: use inline \xff byte rather than %c, add comment > --- > tests/libqtest.c | 8 ++++++++ > tests/test-qga.c | 34 +++++++++++++++++++++++++++------- > 2 files changed, 35 insertions(+), 7 deletions(-) > > diff --git a/tests/libqtest.c b/tests/libqtest.c > index 512c150..84ecbd2 100644 > --- a/tests/libqtest.c > +++ b/tests/libqtest.c > @@ -446,6 +446,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 c780f00..438c2e7 100644 > --- a/tests/test-qga.c > +++ b/tests/test-qga.c > @@ -146,14 +146,23 @@ static void test_qga_sync_delimited(gconstpointer fix) > QDict *ret; > gchar *cmd; > > - cmd = g_strdup_printf("%c{'execute': 'guest-sync-delimited'," > - " 'arguments': {'id': %u } }", 0xff, r); > + 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. > + * > + * TODO: The server shouldn't emit so much garbage (among other > + * things, it loudly complains about the client's \xff being > + * invalid JSON, even though it is a documented part of the > + * handshake. > + */ > + 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 +181,19 @@ static void test_qga_sync(gconstpointer fix) > QDict *ret; > gchar *cmd; > > - cmd = g_strdup_printf("%c{'execute': 'guest-sync'," > - " 'arguments': {'id': %u } }", 0xff, r); > + /* > + * TODO guest-sync is inherently limited: we cannot distinguish > + * failure caused by reacting to garbage on the wire prior to this > + * command, from failure of this actual command. Clients are > + * supposed to be able to send a raw '\xff' byte to at least > + * re-synchronize the server's parser prior to this command, but > + * we are not in a position to test that here because (at least > + * for now) it causes the server to issue an error message about > + * invalid JSON. Testing of '\xff' handling is done in > + * guest-sync-delimited instead. > + */ > + cmd = g_strdup_printf("{'execute': 'guest-sync'," > + " 'arguments': {'id': %u } }", r); > ret = qmp_fd(fixture->fd, cmd); > g_free(cmd); > > -- > 2.9.3 > >
Quoting Michael Roth (2017-05-02 11:46:36) > Quoting Eric Blake (2017-04-27 16:58:21) > > 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. > > The full re-sync sequence is actually to perform that process, > check if the response matches the client-provided sync token, > and then repeat the procedure if it doesn't (in the odd case > that a previous client initiated a guest-sync-delimited > sequence and never consumed the response). The current > implementation only performs one iteration so it's not quite > a full implementation of the documentation procedure. Well, to be more accurate it's a full implementation of the documented procedure, it's just that the procedure isn't fully documented properly. I'll send a patch to address that. > > For the immediate purpose of improving the handling to deal > with the 0xFF-generated error the patch seems sound though, > maybe just something worth noting in the commit msg or > comments so that we might eventually test the full procedure. > > In any case: > > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > > > > 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: Markus Armbruster <armbru@redhat.com> > > > > --- > > v5: add R-b > > v4: no change > > v3: use inline \xff byte rather than %c, add comment > > --- > > tests/libqtest.c | 8 ++++++++ > > tests/test-qga.c | 34 +++++++++++++++++++++++++++------- > > 2 files changed, 35 insertions(+), 7 deletions(-) > > > > diff --git a/tests/libqtest.c b/tests/libqtest.c > > index 512c150..84ecbd2 100644 > > --- a/tests/libqtest.c > > +++ b/tests/libqtest.c > > @@ -446,6 +446,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 c780f00..438c2e7 100644 > > --- a/tests/test-qga.c > > +++ b/tests/test-qga.c > > @@ -146,14 +146,23 @@ static void test_qga_sync_delimited(gconstpointer fix) > > QDict *ret; > > gchar *cmd; > > > > - cmd = g_strdup_printf("%c{'execute': 'guest-sync-delimited'," > > - " 'arguments': {'id': %u } }", 0xff, r); > > + 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. > > + * > > + * TODO: The server shouldn't emit so much garbage (among other > > + * things, it loudly complains about the client's \xff being > > + * invalid JSON, even though it is a documented part of the > > + * handshake. > > + */ > > + 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 +181,19 @@ static void test_qga_sync(gconstpointer fix) > > QDict *ret; > > gchar *cmd; > > > > - cmd = g_strdup_printf("%c{'execute': 'guest-sync'," > > - " 'arguments': {'id': %u } }", 0xff, r); > > + /* > > + * TODO guest-sync is inherently limited: we cannot distinguish > > + * failure caused by reacting to garbage on the wire prior to this > > + * command, from failure of this actual command. Clients are > > + * supposed to be able to send a raw '\xff' byte to at least > > + * re-synchronize the server's parser prior to this command, but > > + * we are not in a position to test that here because (at least > > + * for now) it causes the server to issue an error message about > > + * invalid JSON. Testing of '\xff' handling is done in > > + * guest-sync-delimited instead. > > + */ > > + cmd = g_strdup_printf("{'execute': 'guest-sync'," > > + " 'arguments': {'id': %u } }", r); > > ret = qmp_fd(fixture->fd, cmd); > > g_free(cmd); > > > > -- > > 2.9.3 > > > >
Michael Roth <mdroth@linux.vnet.ibm.com> writes: > Quoting Michael Roth (2017-05-02 11:46:36) >> Quoting Eric Blake (2017-04-27 16:58:21) >> > 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. >> >> The full re-sync sequence is actually to perform that process, >> check if the response matches the client-provided sync token, >> and then repeat the procedure if it doesn't (in the odd case >> that a previous client initiated a guest-sync-delimited >> sequence and never consumed the response). The current >> implementation only performs one iteration so it's not quite >> a full implementation of the documentation procedure. > > Well, to be more accurate it's a full implementation of the > documented procedure, it's just that the procedure isn't > fully documented properly. I'll send a patch to address that. Good. >> For the immediate purpose of improving the handling to deal >> with the 0xFF-generated error the patch seems sound though, >> maybe just something worth noting in the commit msg or >> comments so that we might eventually test the full procedure. Feel free to suggest something for me to add to the commit message. >> In any case: >> >> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> Noted, thanks!
Quoting Markus Armbruster (2017-05-03 03:57:41) > Michael Roth <mdroth@linux.vnet.ibm.com> writes: > > > Quoting Michael Roth (2017-05-02 11:46:36) > >> Quoting Eric Blake (2017-04-27 16:58:21) > >> > 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. > >> > >> The full re-sync sequence is actually to perform that process, > >> check if the response matches the client-provided sync token, > >> and then repeat the procedure if it doesn't (in the odd case > >> that a previous client initiated a guest-sync-delimited > >> sequence and never consumed the response). The current > >> implementation only performs one iteration so it's not quite > >> a full implementation of the documentation procedure. > > > > Well, to be more accurate it's a full implementation of the > > documented procedure, it's just that the procedure isn't > > fully documented properly. I'll send a patch to address that. > > Good. > > >> For the immediate purpose of improving the handling to deal > >> with the 0xFF-generated error the patch seems sound though, > >> maybe just something worth noting in the commit msg or > >> comments so that we might eventually test the full procedure. > > Feel free to suggest something for me to add to the commit message. Maybe change: "which matches the documented actions that a real QGA client is supposed to do." to "which is compatible with the documented actions that a real QGA client is supposed to do." and add the following comment to test_qga_sync_delimited /* * Note that the full reset sequence would involve checking the * response of guest-sync-delimited and repeating the loop if * 'id' field of the response does not match the 'id' field of * the request. Testing this fully would require inserting * garbage in the response stream and is left as a future test * to implement. */ > > >> In any case: > >> > >> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > Noted, thanks! >
Michael Roth <mdroth@linux.vnet.ibm.com> writes: > Quoting Markus Armbruster (2017-05-03 03:57:41) >> Michael Roth <mdroth@linux.vnet.ibm.com> writes: >> >> > Quoting Michael Roth (2017-05-02 11:46:36) >> >> Quoting Eric Blake (2017-04-27 16:58:21) >> >> > 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. >> >> >> >> The full re-sync sequence is actually to perform that process, >> >> check if the response matches the client-provided sync token, >> >> and then repeat the procedure if it doesn't (in the odd case >> >> that a previous client initiated a guest-sync-delimited >> >> sequence and never consumed the response). The current >> >> implementation only performs one iteration so it's not quite >> >> a full implementation of the documentation procedure. >> > >> > Well, to be more accurate it's a full implementation of the >> > documented procedure, it's just that the procedure isn't >> > fully documented properly. I'll send a patch to address that. >> >> Good. >> >> >> For the immediate purpose of improving the handling to deal >> >> with the 0xFF-generated error the patch seems sound though, >> >> maybe just something worth noting in the commit msg or >> >> comments so that we might eventually test the full procedure. >> >> Feel free to suggest something for me to add to the commit message. > > Maybe change: > > "which matches the documented actions that a real QGA client > is supposed to do." > > to > > "which is compatible with the documented actions that a real > QGA client is supposed to do." > > and add the following comment to test_qga_sync_delimited > > /* > * Note that the full reset sequence would involve checking the > * response of guest-sync-delimited and repeating the loop if > * 'id' field of the response does not match the 'id' field of > * the request. Testing this fully would require inserting > * garbage in the response stream and is left as a future test > * to implement. > */ Eric, want me to squash that in?
On 05/04/2017 02:23 AM, Markus Armbruster wrote: >>> Feel free to suggest something for me to add to the commit message. >> >> Maybe change: >> >> "which matches the documented actions that a real QGA client >> is supposed to do." >> >> to >> >> "which is compatible with the documented actions that a real >> QGA client is supposed to do." >> >> and add the following comment to test_qga_sync_delimited >> >> /* >> * Note that the full reset sequence would involve checking the >> * response of guest-sync-delimited and repeating the loop if >> * 'id' field of the response does not match the 'id' field of >> * the request. Testing this fully would require inserting >> * garbage in the response stream and is left as a future test >> * to implement. >> */ > > Eric, want me to squash that in? Yes, those changes are reasonable.
diff --git a/tests/libqtest.c b/tests/libqtest.c index 512c150..84ecbd2 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -446,6 +446,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 c780f00..438c2e7 100644 --- a/tests/test-qga.c +++ b/tests/test-qga.c @@ -146,14 +146,23 @@ static void test_qga_sync_delimited(gconstpointer fix) QDict *ret; gchar *cmd; - cmd = g_strdup_printf("%c{'execute': 'guest-sync-delimited'," - " 'arguments': {'id': %u } }", 0xff, r); + 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. + * + * TODO: The server shouldn't emit so much garbage (among other + * things, it loudly complains about the client's \xff being + * invalid JSON, even though it is a documented part of the + * handshake. + */ + 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 +181,19 @@ static void test_qga_sync(gconstpointer fix) QDict *ret; gchar *cmd; - cmd = g_strdup_printf("%c{'execute': 'guest-sync'," - " 'arguments': {'id': %u } }", 0xff, r); + /* + * TODO guest-sync is inherently limited: we cannot distinguish + * failure caused by reacting to garbage on the wire prior to this + * command, from failure of this actual command. Clients are + * supposed to be able to send a raw '\xff' byte to at least + * re-synchronize the server's parser prior to this command, but + * we are not in a position to test that here because (at least + * for now) it causes the server to issue an error message about + * invalid JSON. Testing of '\xff' handling is done in + * guest-sync-delimited instead. + */ + cmd = g_strdup_printf("{'execute': 'guest-sync'," + " 'arguments': {'id': %u } }", r); ret = qmp_fd(fixture->fd, cmd); g_free(cmd);