Re: [Qemu-devel] [PATCH] monitor: fix expected qmp_capabilities error description regression

[Peter Xu <peterx@redhat.com>]

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.lureau@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