| Message ID | 02aa01d09f9a$5bb198d0$1314ca70$@samsung.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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> 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.
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?
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
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
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 > >
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 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(+)