Eric Blake <ebl...@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.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),
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 <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. Working on it.
