From: Markus Armbruster <armbru@redhat.com>
References: <1432294585-5984-1-git-send-email-armbru@redhat.com>
	<1432294585-5984-2-git-send-email-armbru@redhat.com>
	<555F9C28.4040205@redhat.com>
Date: Tue, 26 May 2015 11:00:37 +0200
In-Reply-To: <555F9C28.4040205@redhat.com> (Eric Blake's message of "Fri, 22
	May 2015 15:14:16 -0600")
Message-ID: <87zj4remru.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH 01/20] monitor: Drop broken,
	unused asynchronous command interface
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, lcapitulino@redhat.com

Eric Blake <eblake@redhat.com> writes:

> 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.

Can do.

>>     {"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 <armbru@redhat.com>
>> ---
>>  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 <eblake@redhat.com>

Thanks!