[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 10/10] test-qga: Actually test 0xff sync byte
From: |
Michael Roth |
Subject: |
Re: [Qemu-devel] [PATCH v5 10/10] test-qga: Actually test 0xff sync bytes |
Date: |
Wed, 03 May 2017 14:52:42 -0500 |
User-agent: |
alot/0.5.1 |
Quoting Markus Armbruster (2017-05-03 03:57:41)
> Michael Roth <address@hidden> 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 <address@hidden>
>
> Noted, thanks!
>