Do not fail if id field is present.
PATCH 1/1 Do not fail if id field is present.
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.html
--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)
monitor.c +2 -0
@@ -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 ;
Signature · 1 line
1.9.5.msysgit.0
Quoted 13 lines · Pavel Fedin
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.htmlSigned-off-by Pavel Fedin <[email protected]>--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)monitor.c +0
-0
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4962
Quoted 8 lines
monitor.c +2
-0@@ -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
Signature · 4 lines
|: 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 :|
Eric Blake
Tested-byQuoted 18 lines · Daniel P. Berrange
Quoted 17 lines · Pavel Fedin
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.htmlSigned-off-by Pavel Fedin <[email protected]>--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)monitor.c +1
-0@@ -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" )) {
Quoted 8 lines
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4962
Quoted 4 lines
monitor.c +1
-0+/* 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.txtto 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:
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 37 lines · Eric Blake
Quoted 18 lines · Daniel P. Berrange
Quoted 17 lines · Pavel Fedin
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.htmlSigned-off-by Pavel Fedin <[email protected]>--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)monitor.c +1
-0@@ -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" )) {Quoted 8 lines
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4962
Quoted 4 lines
monitor.c +1
-0+/* 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.txtto say this is ignored and remove the bit about it being copied into the repliesNot 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}
Quoted 7 lines
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.
Quoted 9 lines
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 */
Quoted 7 lines
At any rate, with the if statement restored, I can now state:
Tested-by Eric Blake <[email protected]>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.
Quoted 9 lines · Markus Armbruster
Quoted 7 lines
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
Quoted 14 lines · Pavel Fedin
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.htmlSigned-off-by Pavel Fedin <[email protected]>--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)monitor.c +0
-0
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4962
Quoted 8 lines
monitor.c +2
-0@@ -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
Quoted 9 lines · Paolo Bonzini
Quoted 8 lines · Pavel Fedin
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.htmlSigned-off-by Pavel Fedin <[email protected]>--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)
Quoted 16 lines
Quoted 6 lines
monitor.c +0
-0monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4962
Quoted 8 lines
monitor.c +2
-0@@ -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?
Quoted 9 lines · Markus Armbruster
Quoted 8 lines
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4962
Quoted 6 lines
monitor.c +2
-0+ }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
Quoted 10 lines · Paolo Bonzini
Quoted 7 lines · Markus Armbruster
Quoted 7 lines
Quoted 6 lines
monitor.c +2
-0+ }else if (!strcmp (arg_name,"id" )) { +/* Ignored, necessary for backwards compatibility */ }else {error_set (errp,QERR_QMP_EXTRA_MEMBER , arg_name);return NULL ;
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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
Quoted 4 lines
Paolo
Quoted 13 lines · Pavel Fedin
This fixes QMP regression:
http://lists.gnu.org/archive/html/qemu-devel/2015-06/msg01795.htmlSigned-off-by Pavel Fedin <[email protected]>--- monitor.c | 2 ++ 1 file changed, 2 insertions(+)monitor.c +0
-0
monitor.c static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp) · line 4959
Quoted 5 lines
monitor.c +2
-0@@ -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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org