[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Do not fail if id field is present.
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH] Do not fail if id field is present. |
Date: |
Fri, 05 Jun 2015 16:10:00 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 |
On 06/05/2015 08:32 AM, Daniel P. Berrange wrote:
> On Fri, Jun 05, 2015 at 05:17:29PM +0300, Pavel Fedin wrote:
>> This fixes QMP regression:
>> http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.html
>>
>> Signed-off-by: Pavel Fedin <address@hidden>
>> ---
>> monitor.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c7baa91..ef21bba 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4955,6 +4955,8 @@ static QDict *qmp_check_input_obj(QObject *input_obj,
>> Error **errp)
>> "arguments", "object");
>> return NULL;
>> }
>> + } else if (!strcmp(arg_name, "id")) {
>> + /* Ignored, necessary for backwards compatibility */
>> } else {
>> error_set(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
>> return NULL;
>
> This should probably be accompanied by an update to docs/qmp/qmp-spec.txt
> to say this is ignored and remove the bit about it being copied into the
> replies
Not necessary - the "id" string is once again preserved correctly on
synchronous commands with this patch applied:
$ ./x86_64-softmmu/qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 3, "major": 2},
"package": ""}, "capabilities": []}}
{"execute":"qmp_capabilities","id":1}
{"return": {}, "id": 1}
Remember, Markus' patch was about removing asynchronous commands,
because _those_ commands were where "id" was mishandled (and if we DID
want asynch commands, it would be even MORE important that they handle
id corerctly). But he accidentally removed 2 lines (the strcmp to "id"
and a now stale FIXME comment about wanting to check for duplicate "id"s
being tracked across parallel async commands) when it should have only
removed one (the stale comment), and I made the mistake of giving R-by
based on code review and NOT an actual build-and-run of the applied
patch (or I would have instantly spotted the side effect that "id" was
broken on synchronous commands). But an empty if clause looks
suspicious, so we want a better comment in place of the stale comment,
which is why I suggest:
/* Specially handled elsewhere to be included in reply to user */
At any rate, with the if statement restored, I can now state:
Tested-by: Eric Blake <address@hidden>
and since we now have no less than three threads pointing out the issue,
I hope we can settle on a solution with a nice comment and get it in the
tree soon.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] Do not fail if id field is present., Eric Blake, 2015/06/05