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

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

[Marc-André Lureau]

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'");
+            }
+        }
+
         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

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

[Peter Xu]

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

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

[Marc-André Lureau]

Hi

On Fri, Mar 23, 2018 at 4:35 PM, Peter Xu <peterx@redhat.com> 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 <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?
>

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

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

[Eric Blake]

On 03/23/2018 10:50 AM, Marc-André Lureau wrote:

>> 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?

Yes, that looks nicer.

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

There have been quite a few patch ideas across multiple threads related 
to OOB fallout.  Hopefully I can keep straight which patches are 
intended for 2.12 (anything that fixes a bug, like this one, is a good 
candidate, and it would be nice if we can undo the temporary reversion 
of exposing OOB if we can solve all the issues that iotests exposed).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Peter Xu]

On Fri, Mar 23, 2018 at 04:56:34PM -0500, Eric Blake wrote:

[...]

> > > 
> > 
> > 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.
> 
> There have been quite a few patch ideas across multiple threads related to
> OOB fallout.  Hopefully I can keep straight which patches are intended for
> 2.12 (anything that fixes a bug, like this one, is a good candidate,

I'll mark patches with "for-2.12" if there are.

> and it
> would be nice if we can undo the temporary reversion of exposing OOB if we
> can solve all the issues that iotests exposed).

IMHO it'll still be risky considering what has already reported.

Here's my plan, hopefully to make everyone happy - we keep OOB turned
off for 2.12 and even later.  In 2.13, I'll post some new patches to
add a new monitor parameter to allow user to enable OOB explicitly,
otherwise we never enable it.  After all, for now the only real user
should be postcopy. Then we don't need to struggle around all these
mess.  What do you think?

-- 
Peter Xu

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

[Eric Blake]

On 03/23/2018 08:41 PM, Peter Xu wrote:

>> There have been quite a few patch ideas across multiple threads related to
>> OOB fallout.  Hopefully I can keep straight which patches are intended for
>> 2.12 (anything that fixes a bug, like this one, is a good candidate,
> 
> I'll mark patches with "for-2.12" if there are.
> 
>> and it
>> would be nice if we can undo the temporary reversion of exposing OOB if we
>> can solve all the issues that iotests exposed).
> 
> IMHO it'll still be risky considering what has already reported.
> 
> Here's my plan, hopefully to make everyone happy - we keep OOB turned
> off for 2.12 and even later.  In 2.13, I'll post some new patches to
> add a new monitor parameter to allow user to enable OOB explicitly,
> otherwise we never enable it.  After all, for now the only real user
> should be postcopy. Then we don't need to struggle around all these
> mess.  What do you think?

If you're going to add a CLI parameter that must be specified for OOB to 
even be advertised, then it is MUCH less invasive to existing clients 
(it does mean that opting in to OOB now requires the command line 
argument AND the capability request during qmp_capabilities) - as such, 
enabling the opt-in during 2.12 is less controversial, and I see no 
reason to defer it to 2.13, especially if you want to maximize testing 
of the new feature to shake out the bugs it encounters.

If you want to be cautious, name the command-line parameter --x-oob for 
now, we can rename it later to drop the x- prefix, or remove the 
parameter altogether if we decide by opting in via merely 
qmp_capabilities is sufficient.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Peter Xu]

On Sat, Mar 24, 2018 at 06:41:04AM -0500, Eric Blake wrote:
> On 03/23/2018 08:41 PM, Peter Xu wrote:
> 
> > > There have been quite a few patch ideas across multiple threads related to
> > > OOB fallout.  Hopefully I can keep straight which patches are intended for
> > > 2.12 (anything that fixes a bug, like this one, is a good candidate,
> > 
> > I'll mark patches with "for-2.12" if there are.
> > 
> > > and it
> > > would be nice if we can undo the temporary reversion of exposing OOB if we
> > > can solve all the issues that iotests exposed).
> > 
> > IMHO it'll still be risky considering what has already reported.
> > 
> > Here's my plan, hopefully to make everyone happy - we keep OOB turned
> > off for 2.12 and even later.  In 2.13, I'll post some new patches to
> > add a new monitor parameter to allow user to enable OOB explicitly,
> > otherwise we never enable it.  After all, for now the only real user
> > should be postcopy. Then we don't need to struggle around all these
> > mess.  What do you think?
> 
> If you're going to add a CLI parameter that must be specified for OOB to
> even be advertised, then it is MUCH less invasive to existing clients (it
> does mean that opting in to OOB now requires the command line argument AND
> the capability request during qmp_capabilities) - as such, enabling the
> opt-in during 2.12 is less controversial, and I see no reason to defer it to
> 2.13, especially if you want to maximize testing of the new feature to shake
> out the bugs it encounters.
> 
> If you want to be cautious, name the command-line parameter --x-oob for now,
> we can rename it later to drop the x- prefix, or remove the parameter
> altogether if we decide by opting in via merely qmp_capabilities is
> sufficient.

Hmm, it seems I don't even need to wait. :-)

I'll prepare something soon (together with some existing known fixes).

Thanks!

-- 
Peter Xu