On Fri, Mar 23, 2018 at 11:32:39AM +0100, Marc-André Lureau wrote:
> Fix regression introduced in commit cf869d53172920536a14180a83292b240e9d0545:
>
> Before:
> {"execute":"foo"}
> {"error": {"class": "CommandNotFound", "desc": "Expecting capabilities
> negotiation with 'qmp_capabilities'"}}
>
> After:
> {"execute":"foo"}
> {"error": {"class": "CommandNotFound", "desc": "The command foo has not been
> found"}}
>
> Because qmp_cmd_oob_check() happens before
> monitor_qmp_dispatch_one(). Move the error manipulation code to
> monitor_qmp_respond() instead.
>
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> monitor.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 6ccd2fc089..d57bbbb134 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3997,6 +3997,18 @@ static void monitor_qmp_respond(Monitor *mon, QObject
> *rsp,
> qdict_put_obj(qobject_to(QDict, rsp), "id", id);
> }
>
> + if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> + qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> + if (qdict
> + && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> +
> QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
> + /* Provide a more useful error message */
> + qdict_del(qdict, "desc");
> + qdict_put_str(qdict, "desc", "Expecting capabilities"
> + " negotiation with 'qmp_capabilities'");
> + }
> + }
> +
If we are going to remove below chunk, how about do it in prettier
way instead of hacking around the error again? Like:
diff --git a/monitor.c b/monitor.c
index 77f4c41cfa..849fa23bf9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1203,8 +1203,14 @@ static bool qmp_cmd_oob_check(Monitor *mon, QDict *req,
Error **errp)
cmd = qmp_find_command(mon->qmp.commands, command);
if (!cmd) {
- error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
- "The command %s has not been found", command);
+ if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
+ error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+ "Expecting capabilities negotiation "
+ "with 'qmp_capabilities'");
+ } else {
+ error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+ "The command %s has not been found", command);
+ }
return false;
}
What do you think?
Thanks,
> monitor_json_emitter(mon, rsp);
> }
>
> @@ -4028,7 +4040,6 @@ static void monitor_qmp_dispatch_one(QMPRequest
> *req_obj)
> {
> Monitor *mon, *old_mon;
> QObject *req, *rsp = NULL, *id;
> - QDict *qdict = NULL;
> bool need_resume;
>
> req = req_obj->req;
> @@ -4051,18 +4062,6 @@ static void monitor_qmp_dispatch_one(QMPRequest
> *req_obj)
>
> cur_mon = old_mon;
>
> - if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
> - qdict = qdict_get_qdict(qobject_to(QDict, rsp), "error");
> - if (qdict
> - && !g_strcmp0(qdict_get_try_str(qdict, "class"),
> - QapiErrorClass_str(ERROR_CLASS_COMMAND_NOT_FOUND))) {
> - /* Provide a more useful error message */
> - qdict_del(qdict, "desc");
> - qdict_put_str(qdict, "desc", "Expecting capabilities negotiation"
> - " with 'qmp_capabilities'");
> - }
> - }
> -
> /* Respond if necessary */
> monitor_qmp_respond(mon, rsp, NULL, id);
>
> --
> 2.17.0.rc1.1.g4c4f2b46a3
>
--
Peter Xu
