Re: [Qemu-devel] [PATCH] Do not fail if id field is present.

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

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 <p.fedin@samsung.com>
>> ---
>>  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 <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]