Re: [Qemu-devel] [PATCH v5 10/10] test-qga: Actually test 0xff sync bytes

[Markus Armbruster]

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.

>> In any case:
>> 
>> Reviewed-by: Michael Roth <address@hidden>

Noted, thanks!