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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> 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),

Exactly.

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

Working on it.