qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Marc-André Lureau
Subject: Re: [Qemu-devel] [PATCH] monitor: fix expected qmp_capabilities error description regression
Date: Fri, 23 Mar 2018 16:50:28 +0100

Hi

On Fri, Mar 23, 2018 at 4:35 PM, Peter Xu <address@hidden> wrote:
> 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 <address@hidden>
>> ---
>>  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?
>

Works for me (fwiw, I'll probably need the replace "hack" again,
because in the RFC series I am about to send, the code is factored out
/ generalized in qmp-dispatch), but that works in the meantime, please
send a patch.

> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]