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

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

[Pavel Fedin]

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;
-- 
1.9.5.msysgit.0

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

[Daniel P. Berrange]

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


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Eric Blake]

[-- 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 --]

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

[Markus Armbruster]

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.

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

[Paolo Bonzini]

On 08/06/2015 10:08, Markus Armbruster wrote:
> > 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.

Oh, then it does fix virt-test too.

Paolo

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

[Paolo Bonzini]

On 05/06/2015 16:17, 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;
> 

Nope, this is not enough to fix virt-test.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/06/2015 16:17, 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;
>> 
>
> Nope, this is not enough to fix virt-test.

Really?  Details?

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

[Paolo Bonzini]

On 08/06/2015 10:10, Markus Armbruster wrote:
> > > +        } else if (!strcmp(arg_name, "id")) {
> > > +            /* Ignored, necessary for backwards compatibility */
> > >          } else {
> > >              error_set(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
> > >              return NULL;
> > > 
> >
> > Nope, this is not enough to fix virt-test.
> Really?  Details?

It really wants the id in the reply to match the id in the request.

Paolo

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

[Wen Congyang]

On 06/08/2015 04:17 PM, Paolo Bonzini wrote:
> 
> 
> On 08/06/2015 10:10, Markus Armbruster wrote:
>>>> +        } else if (!strcmp(arg_name, "id")) {
>>>> +            /* Ignored, necessary for backwards compatibility */
>>>>          } else {
>>>>              error_set(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
>>>>              return NULL;
>>>>
>>>
>>> Nope, this is not enough to fix virt-test.
>> Really?  Details?
> 
> It really wants the id in the reply to match the id in the request.

In handle_qmp_command():
    mon->qmp.id = qdict_get(input, "id");
    qobject_incref(mon->qmp.id);
In monitor_protocol_emitter():
    if (mon->qmp.id) {
        qdict_put_obj(qmp, "id", mon->qmp.id);
        mon->qmp.id = NULL;
    }

So I think there is no problems for virt-test.

Thanks
Wen Congyang

> 
> Paolo
> 
> 

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

[Eric Blake]

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

On 06/05/2015 08:17 AM, 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 */

I think a better comment would be:

/* Specially handled elsewhere to be included in reply to user */

But I agree that we need something like this patch to avoid a regression
of causing QERR_QMP_EXTRA_MEMBER, since "id" is a documented part of the
protocol.

We also have other patches floating for the same issue:
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01488.html

that differ only in the choice of comment.

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