Re: [Qemu-devel] [Xen-devel] qemu mainline regression with xen-unstable: unable to start QMP

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> [adding Markus, as author of the regression]
>
> On 06/04/2015 03:59 PM, Don Slutz wrote:
>> On 06/04/15 11:04, Fabio Fantoni wrote:
>>> Today after trying xen-unstable build (tested many hours) of some days
>>> ago I tried update qemu to latest development version (from git master
>>> commit 6fa6b312765f698dc81b2c30e7eeb9683804a05b) and seems that there is
>>> a regression:
>>>> xl create /etc/xen/W7.cfg
>>>> Parsing config from /etc/xen/W7.cfg
>>>> libxl: error: libxl_qmp.c:287:qmp_handle_error_response: received an
>>>> error message from QMP server: QMP input object member 'id' is unexpected
>>>> libxl: error: libxl_qmp.c:715:libxl__qmp_initialize: Failed to connect
>>>> to QMP
>>>
>> 
>> This is caused by:
>> 
>> commit 65207c59d99f2260c5f1d3b9c491146616a522aa
>> Author: Markus Armbruster <address@hidden>
>> Date:   Thu Mar 5 14:35:26 2015 +0100
>> 
>>     monitor: Drop broken, unused asynchronous command interface

Yes.  I screwed up.

>> The patch:
>> 
>>>From 1b0221078353870fe530e49de158cae205f9bce5 Mon Sep 17 00:00:00 2001
>> From: Don Slutz <address@hidden>
>> Date: Thu, 4 Jun 2015 17:04:42 -0400
>> Subject: [PATCH 01/14] monitor: Allow Xen's (broken) usage of asynchronous
>>  command interface.
>> 
>> commit 65207c59d99f2260c5f1d3b9c491146616a522aa
>> Author: Markus Armbruster <address@hidden>
>> Date:   Thu Mar 5 14:35:26 2015 +0100
>> 
>>     monitor: Drop broken, unused asynchronous command interface
>> 
>> Breaks Xen.  Add a hack un unbreak it.
>
> s/un /to /
>
>> 
>> Xen is only doing synchronous commands, but is including an id.
>
> QMP also uses id, but apparently removes it up front before calling into
> this function; so another fix would be having xen remove it up front.

I don't think so:

    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 3, "major": 2}, 
"package": ""}, "capabilities": []}}
    { "execute": "qmp_capabilities" }
    {"return": {}}
    {"execute": "system_reset", "id": "1"}
    {"error": {"class": "GenericError", "desc": "QMP input object member 'id' 
is unexpected"}}

>> Signed-off-by: Don Slutz <address@hidden>
>> ---
>>  monitor.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index c7baa91..e9a0747 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4955,6 +4955,15 @@ static QDict *qmp_check_input_obj(QObject
>> *input_obj, Error **errp)
>>                            "arguments", "object");
>>                  return NULL;
>>              }
>> +        } else if (!strcmp(arg_name, "id")) {
>> +            /*
>> +             * Fixup Xen's usage. Just ignore the "id". See point #5
>> +             * above.  This was an attempt at an asynchronous
>> +             * command interface.  However commit
>> +             * 65207c59d99f2260c5f1d3b9c491146616a522aa is
>> +             * wrong. Xen does not expect an error when it passes in
>> +             * "id":1, so just continue to ignore it.
>> +             */
>
> The comment is a bit verbose, particularly since 'id' is a
> well-established usage pattern in QMP.  Also, we don't need to call out
> why it changed in the comment here, the commit message is sufficient for
> that.

Yes.  I'll post a patch with a more suitable comment and commit message.

>>          } else {
>>              error_set(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
>>              return NULL;
>> 

I apologize for the mess I made, and my slow reply (offline since
Wednesday night).