Re: [Qemu-devel] [PATCH 01/20] monitor: Drop broken, unused asynchronous command interface

[Eric Blake]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> The asynchronous monitor command interface goes back to commit 940cc30
> (Jan 2010).  Added a third case to command execution.  The hope back
> then according to the commit message was that all commands get
> converted to the asynchronous interface, killing off the other two
> cases.  Didn't happen.
> 
> The initial asynchronous commands balloon and info balloon were
> converted back to synchronous long ago (commit 96637bc and d72f32),
> with commit messages calling the asynchronous interface "not fully
> working" and "deprecated".  The only other user went away in commit
> 3b5704b.

The history is helpful; thanks.

> 
> New code generally uses synchronous commands and asynchronous events.
> 
> What exactly is still "not fully working" with asynchronous commands?
> Well, here's a bug that defeats actual asynchronous use pretty
> reliably: the reply's ID is wrong (and has always been wrong) unless
> you use the command synchronously!  To reproduce, we need an
> asynchronous command, so we have to go back before the commit 3b5704b.
> Run QEMU with spice:
> 
>     $ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing 
> -qmp stdio
>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major":
> 2}, "package": ""}, "capabilities": []}}
> 
> Connect a spice client in another terminal:
> 
>     $ remote-viewer spice://localhost:5900
> 
> Set up a migration destination dummy in a third terminal:
> 
>     $ socat TCP-LISTEN:12345 STDIO
> 
> Now paste the following into the QMP monitor:
> 
>     { "execute": "qmp_capabilities" }

If you stick and "id":"i0" here...

>     { "execute": "client_migrate_info", "id": "i1", "arguments": { 
> "protocol": "spice", "hostname": "localhost", "port": 12345 } }
>     { "execute": "query-kvm", "id": "i2" }
> 
> Produces two replies immediately, one to qmp_capabilities, and one to
> query-kvm:
> 
>     {"return": {}}

...it should show up here, for even less ambiguity about the problems
with the async response.

>     {"return": {"enabled": false, "present": true}, "id": "i2"}
> 
> Both are correct.  Two lines of debug output from libspice-server not
> shown.
> 
> Now EOF socat's standard input to make it close the connection.  This
> makes the asynchronous client_migrate_info complete.  It replies:
> 
>     {"return": {}}
> 
> Bug: "id": "i1" is missing.  Two lines of debug output from
> libspice-server not shown.  Cherry on top: storage for the missing ID
> is leaked.
> 
> Get rid of this stuff before somebody hurts himself with it.
> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  include/monitor/monitor.h |  5 ----
>  monitor.c                 | 68 
> ++---------------------------------------------
>  2 files changed, 2 insertions(+), 71 deletions(-)

Certainly takes an axe to it!

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature