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.fe...@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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
