[Qemu-devel] [PATCH 0/3] QMP: small fixes

[Qemu-devel] [PATCH 0/3] QMP: small fixes

[Luiz Capitulino]

Please, check individual patches for details.

[Qemu-devel] [PATCH 1/3] QMP: Fix default response regression

[Luiz Capitulino]

Commit 030db6e89d dropped do_info() usage from QMP and introduced
qmp_call_query_cmd(). However, the new function doesn't emit QMP's
default OK response when the handler doesn't return data.

Fix that by also calling monitor_protocol_emitter() when
ret_data == NULL, so that the default response is emitted.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 8cee35d..c4efe58 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4426,10 +4426,8 @@ static void qmp_call_query_cmd(Monitor *mon, const mon_cmd_t *cmd)
         }
     } else {
         cmd->mhandler.info_new(mon, &ret_data);
-        if (ret_data) {
-            monitor_protocol_emitter(mon, ret_data);
-            qobject_decref(ret_data);
-        }
+        monitor_protocol_emitter(mon, ret_data);
+        qobject_decref(ret_data);
     }
 }
 
-- 
1.7.3.2.245.g03276

[Qemu-devel] [PATCH 2/3] QMP: Drop dead code

[Luiz Capitulino]

The first if/else clause in handler_audit() makes no sense for two
reasons:

  1. this function is now called only by QMP code, so testing if
     it's a QMP call makes no sense anymore

  2. the else clause first asserts that there's no error in the
     monitor object, then it tries to free it!

Just drop it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |   74 ++++++++++++++++++++++++++++---------------------------------
 1 files changed, 34 insertions(+), 40 deletions(-)

diff --git a/monitor.c b/monitor.c
index c4efe58..30be273 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3853,49 +3853,43 @@ void monitor_set_error(Monitor *mon, QError *qerror)
 
 static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
 {
-    if (monitor_ctrl_mode(mon)) {
-        if (ret && !monitor_has_error(mon)) {
-            /*
-             * If it returns failure, it must have passed on error.
-             *
-             * Action: Report an internal error to the client if in QMP.
-             */
-            qerror_report(QERR_UNDEFINED_ERROR);
-            MON_DEBUG("command '%s' returned failure but did not pass an error\n",
-                      cmd->name);
-        }
+    if (ret && !monitor_has_error(mon)) {
+        /*
+         * If it returns failure, it must have passed on error.
+         *
+         * Action: Report an internal error to the client if in QMP.
+         */
+        qerror_report(QERR_UNDEFINED_ERROR);
+        MON_DEBUG("command '%s' returned failure but did not pass an error\n",
+                  cmd->name);
+    }
 
 #ifdef CONFIG_DEBUG_MONITOR
-        if (!ret && monitor_has_error(mon)) {
-            /*
-             * If it returns success, it must not have passed an error.
-             *
-             * Action: Report the passed error to the client.
-             */
-            MON_DEBUG("command '%s' returned success but passed an error\n",
-                      cmd->name);
-        }
-
-        if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
-            /*
-             * Handlers should not call Monitor print functions.
-             *
-             * Action: Ignore them in QMP.
-             *
-             * (XXX: we don't check any 'info' or 'query' command here
-             * because the user print function _is_ called by do_info(), hence
-             * we will trigger this check. This problem will go away when we
-             * make 'query' commands real and kill do_info())
-             */
-            MON_DEBUG("command '%s' called print functions %d time(s)\n",
-                      cmd->name, mon_print_count_get(mon));
-        }
-#endif
-    } else {
-        assert(!monitor_has_error(mon));
-        QDECREF(mon->error);
-        mon->error = NULL;
+    if (!ret && monitor_has_error(mon)) {
+        /*
+         * If it returns success, it must not have passed an error.
+         *
+         * Action: Report the passed error to the client.
+         */
+        MON_DEBUG("command '%s' returned success but passed an error\n",
+                  cmd->name);
+    }
+
+    if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
+        /*
+         * Handlers should not call Monitor print functions.
+         *
+         * Action: Ignore them in QMP.
+         *
+         * (XXX: we don't check any 'info' or 'query' command here
+         * because the user print function _is_ called by do_info(), hence
+         * we will trigger this check. This problem will go away when we
+         * make 'query' commands real and kill do_info())
+         */
+        MON_DEBUG("command '%s' called print functions %d time(s)\n",
+                  cmd->name, mon_print_count_get(mon));
     }
+#endif
 }
 
 static void handle_user_command(Monitor *mon, const char *cmdline)
-- 
1.7.3.2.245.g03276

[Qemu-devel] [PATCH 3/3] QMP: Simplify monitor_json_emitter()

[Luiz Capitulino]

Use the ternary operator instead of an if (also fixes bad indentation).

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index 30be273..f6f2264 100644
--- a/monitor.c
+++ b/monitor.c
@@ -351,10 +351,8 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
 {
     QString *json;
 
-    if (mon->flags & MONITOR_USE_PRETTY)
-	json = qobject_to_json_pretty(data);
-    else
-	json = qobject_to_json(data);
+    json = mon->flags & MONITOR_USE_PRETTY ? qobject_to_json_pretty(data) :
+                                             qobject_to_json(data);
     assert(json != NULL);
 
     qstring_append_chr(json, '\n');
-- 
1.7.3.2.245.g03276

Re: [Qemu-devel] [PATCH 2/3] QMP: Drop dead code

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> The first if/else clause in handler_audit() makes no sense for two
> reasons:
>
>   1. this function is now called only by QMP code, so testing if
>      it's a QMP call makes no sense anymore
>
>   2. the else clause first asserts that there's no error in the
>      monitor object, then it tries to free it!
>
> Just drop it.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c |   74 ++++++++++++++++++++++++++++---------------------------------
>  1 files changed, 34 insertions(+), 40 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index c4efe58..30be273 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3853,49 +3853,43 @@ void monitor_set_error(Monitor *mon, QError *qerror)
>  
>  static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
>  {
> -    if (monitor_ctrl_mode(mon)) {
> -        if (ret && !monitor_has_error(mon)) {
> -            /*
> -             * If it returns failure, it must have passed on error.
> -             *
> -             * Action: Report an internal error to the client if in QMP.
> -             */
> -            qerror_report(QERR_UNDEFINED_ERROR);
> -            MON_DEBUG("command '%s' returned failure but did not pass an error\n",
> -                      cmd->name);
> -        }
> +    if (ret && !monitor_has_error(mon)) {
> +        /*
> +         * If it returns failure, it must have passed on error.
> +         *
> +         * Action: Report an internal error to the client if in QMP.
> +         */
> +        qerror_report(QERR_UNDEFINED_ERROR);
> +        MON_DEBUG("command '%s' returned failure but did not pass an error\n",
> +                  cmd->name);
> +    }
>  
>  #ifdef CONFIG_DEBUG_MONITOR
> -        if (!ret && monitor_has_error(mon)) {
> -            /*
> -             * If it returns success, it must not have passed an error.
> -             *
> -             * Action: Report the passed error to the client.
> -             */
> -            MON_DEBUG("command '%s' returned success but passed an error\n",
> -                      cmd->name);
> -        }
> -
> -        if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
> -            /*
> -             * Handlers should not call Monitor print functions.
> -             *
> -             * Action: Ignore them in QMP.
> -             *
> -             * (XXX: we don't check any 'info' or 'query' command here
> -             * because the user print function _is_ called by do_info(), hence
> -             * we will trigger this check. This problem will go away when we
> -             * make 'query' commands real and kill do_info())
> -             */
> -            MON_DEBUG("command '%s' called print functions %d time(s)\n",
> -                      cmd->name, mon_print_count_get(mon));
> -        }
> -#endif
> -    } else {
> -        assert(!monitor_has_error(mon));
> -        QDECREF(mon->error);
> -        mon->error = NULL;
> +    if (!ret && monitor_has_error(mon)) {
> +        /*
> +         * If it returns success, it must not have passed an error.
> +         *
> +         * Action: Report the passed error to the client.
> +         */
> +        MON_DEBUG("command '%s' returned success but passed an error\n",
> +                  cmd->name);
> +    }
> +
> +    if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
> +        /*
> +         * Handlers should not call Monitor print functions.
> +         *
> +         * Action: Ignore them in QMP.
> +         *
> +         * (XXX: we don't check any 'info' or 'query' command here
> +         * because the user print function _is_ called by do_info(), hence
> +         * we will trigger this check. This problem will go away when we
> +         * make 'query' commands real and kill do_info())
> +         */
> +        MON_DEBUG("command '%s' called print functions %d time(s)\n",
> +                  cmd->name, mon_print_count_get(mon));
>      }
> +#endif
>  }
>  
>  static void handle_user_command(Monitor *mon, const char *cmdline)

For easier review:

$ git-diff -w
diff --git a/monitor.c b/monitor.c
index 8cee35d..138db06 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3853,7 +3853,6 @@ void monitor_set_error(Monitor *mon, QError *qerror)
 
 static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
 {
-    if (monitor_ctrl_mode(mon)) {
         if (ret && !monitor_has_error(mon)) {
             /*
              * If it returns failure, it must have passed on error.
@@ -3891,11 +3890,6 @@ static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
                       cmd->name, mon_print_count_get(mon));
         }
 #endif
-    } else {
-        assert(!monitor_has_error(mon));
-        QDECREF(mon->error);
-        mon->error = NULL;
-    }
 }
 
 static void handle_user_command(Monitor *mon, const char *cmdline)

Re: [Qemu-devel] [PATCH 0/3] QMP: small fixes

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Please, check individual patches for details.

ACK series