[Qemu-devel] [PATCH 00/20] monitor: Wean core off QError, and other cleanups

[Qemu-devel] [PATCH 00/20] monitor: Wean core off QError, and other cleanups

[Markus Armbruster]

Command handlers still use QError.  Left for another day.

Markus Armbruster (20):
  monitor: Drop broken, unused asynchronous command interface
  monitor: Clean up after previous commit
  monitor: Improve and document client_migrate_info protocol error
  monitor: Convert client_migrate_info to QAPI
  monitor: Use traditional command interface for HMP drive_del
  monitor: Use traditional command interface for HMP device_add
  monitor: Use trad. command interface for HMP pcie_aer_inject_error
  monitor: Drop unused "new" HMP command interface
  monitor: Propagate errors through qmp_check_client_args()
  monitor: Propagate errors through qmp_check_input_obj()
  monitor: Wean monitor_protocol_emitter() off mon->error
  monitor: Inline monitor_has_error() into its only caller
  monitor: Limit QError use to command handlers
  monitor: Rename handle_user_command() to handle_hmp_command()
  monitor: Rename monitor_control_read(), monitor_control_event()
  monitor: Unbox Monitor member mc and rename to qmp
  monitor: Drop do_qmp_capabilities()'s superfluous QMP check
  monitor: Turn int command_mode into bool in_command_mode
  monitor: Rename monitor_ctrl_mode() to monitor_is_qmp()
  monitor: Change return type of monitor_cur_is_qmp() to bool

 blockdev.c                |   9 +-
 hmp-commands.hx           |  13 +-
 hmp.c                     |  23 +++
 hmp.h                     |   2 +
 hw/pci/pci-stub.c         |  14 +-
 hw/pci/pcie_aer.c         |  39 ++---
 include/monitor/monitor.h |   7 +-
 include/sysemu/blockdev.h |   2 +-
 include/sysemu/sysemu.h   |   4 +-
 monitor.c                 | 378 ++++++++++++++++------------------------------
 qapi-schema.json          |  20 +++
 qmp-commands.hx           |   5 +-
 stubs/mon-is-qmp.c        |   2 +-
 13 files changed, 214 insertions(+), 304 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 01/20] monitor: Drop broken, unused asynchronous command interface

[Markus Armbruster]

The asynchronous monitor command interface goes back to commit 940cc30
(Jan 2010).  Added a third case to command execution.  The hope back
then according to the commit message was that all commands get
converted to the asynchronous interface, killing off the other two
cases.  Didn't happen.

The initial asynchronous commands balloon and info balloon were
converted back to synchronous long ago (commit 96637bc and d72f32),
with commit messages calling the asynchronous interface "not fully
working" and "deprecated".  The only other user went away in commit
3b5704b.

New code generally uses synchronous commands and asynchronous events.

What exactly is still "not fully working" with asynchronous commands?
Well, here's a bug that defeats actual asynchronous use pretty
reliably: the reply's ID is wrong (and has always been wrong) unless
you use the command synchronously!  To reproduce, we need an
asynchronous command, so we have to go back before the commit 3b5704b.
Run QEMU with spice:

    $ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing -qmp stdio
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major":
2}, "package": ""}, "capabilities": []}}

Connect a spice client in another terminal:

    $ remote-viewer spice://localhost:5900

Set up a migration destination dummy in a third terminal:

    $ socat TCP-LISTEN:12345 STDIO

Now paste the following into the QMP monitor:

    { "execute": "qmp_capabilities" }
    { "execute": "client_migrate_info", "id": "i1", "arguments": { "protocol": "spice", "hostname": "localhost", "port": 12345 } }
    { "execute": "query-kvm", "id": "i2" }

Produces two replies immediately, one to qmp_capabilities, and one to
query-kvm:

    {"return": {}}
    {"return": {"enabled": false, "present": true}, "id": "i2"}

Both are correct.  Two lines of debug output from libspice-server not
shown.

Now EOF socat's standard input to make it close the connection.  This
makes the asynchronous client_migrate_info complete.  It replies:

    {"return": {}}

Bug: "id": "i1" is missing.  Two lines of debug output from
libspice-server not shown.  Cherry on top: storage for the missing ID
is leaked.

Get rid of this stuff before somebody hurts himself with it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/monitor/monitor.h |  5 ----
 monitor.c                 | 68 ++---------------------------------------------
 2 files changed, 2 insertions(+), 71 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index df67d56..d409b6a 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,9 +16,6 @@ extern Monitor *default_mon;
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
 
-/* flags for monitor commands */
-#define MONITOR_CMD_ASYNC       0x0001
-
 int monitor_cur_is_qmp(void);
 
 void monitor_init(CharDriverState *chr, int flags);
@@ -43,8 +40,6 @@ void monitor_flush(Monitor *mon);
 int monitor_set_cpu(int cpu_index);
 int monitor_get_cpu_index(void);
 
-typedef void (MonitorCompletion)(void *opaque, QObject *ret_data);
-
 void monitor_set_error(Monitor *mon, QError *qerror);
 void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
diff --git a/monitor.c b/monitor.c
index b2561e1..9a0c3b7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -118,12 +118,6 @@
  *
  */
 
-typedef struct MonitorCompletionData MonitorCompletionData;
-struct MonitorCompletionData {
-    Monitor *mon;
-    void (*user_print)(Monitor *mon, const QObject *data);
-};
-
 typedef struct mon_cmd_t {
     const char *name;
     const char *args_type;
@@ -133,10 +127,7 @@ typedef struct mon_cmd_t {
     union {
         void (*cmd)(Monitor *mon, const QDict *qdict);
         int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
-        int  (*cmd_async)(Monitor *mon, const QDict *params,
-                          MonitorCompletion *cb, void *opaque);
     } mhandler;
-    int flags;
     /* @sub_table is a list of 2nd level of commands. If it do not exist,
      * mhandler should be used. If it exist, sub_table[?].mhandler should be
      * used, and mhandler of 1st level plays the role of help function.
@@ -394,11 +385,6 @@ static inline int handler_is_qobject(const mon_cmd_t *cmd)
     return cmd->user_print != NULL;
 }
 
-static inline bool handler_is_async(const mon_cmd_t *cmd)
-{
-    return cmd->flags & MONITOR_CMD_ASYNC;
-}
-
 static inline int monitor_has_error(const Monitor *mon)
 {
     return mon->error != NULL;
@@ -917,45 +903,6 @@ static void hmp_trace_file(Monitor *mon, const QDict *qdict)
 }
 #endif
 
-static void user_monitor_complete(void *opaque, QObject *ret_data)
-{
-    MonitorCompletionData *data = (MonitorCompletionData *)opaque; 
-
-    if (ret_data) {
-        data->user_print(data->mon, ret_data);
-    }
-    monitor_resume(data->mon);
-    g_free(data);
-}
-
-static void qmp_monitor_complete(void *opaque, QObject *ret_data)
-{
-    monitor_protocol_emitter(opaque, ret_data);
-}
-
-static int qmp_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
-                                 const QDict *params)
-{
-    return cmd->mhandler.cmd_async(mon, params, qmp_monitor_complete, mon);
-}
-
-static void user_async_cmd_handler(Monitor *mon, const mon_cmd_t *cmd,
-                                   const QDict *params)
-{
-    int ret;
-
-    MonitorCompletionData *cb_data = g_malloc(sizeof(*cb_data));
-    cb_data->mon = mon;
-    cb_data->user_print = cmd->user_print;
-    monitor_suspend(mon);
-    ret = cmd->mhandler.cmd_async(mon, params,
-                                  user_monitor_complete, cb_data);
-    if (ret < 0) {
-        monitor_resume(mon);
-        g_free(cb_data);
-    }
-}
-
 static void hmp_info_help(Monitor *mon, const QDict *qdict)
 {
     help_cmd(mon, "info");
@@ -4121,9 +4068,7 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
     if (!cmd)
         goto out;
 
-    if (handler_is_async(cmd)) {
-        user_async_cmd_handler(mon, cmd, qdict);
-    } else if (handler_is_qobject(cmd)) {
+    if (handler_is_qobject(cmd)) {
         QObject *data = NULL;
 
         /* XXX: ignores the error code */
@@ -5134,16 +5079,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    if (handler_is_async(cmd)) {
-        err = qmp_async_cmd_handler(mon, cmd, args);
-        if (err) {
-            /* emit the error response */
-            goto err_out;
-        }
-    } else {
-        qmp_call_cmd(mon, cmd, args);
-    }
-
+    qmp_call_cmd(mon, cmd, args);
     goto out;
 
 err_out:
-- 
1.9.3

[Qemu-devel] [PATCH 02/20] monitor: Clean up after previous commit

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 40 +++++++++++-----------------------------
 1 file changed, 11 insertions(+), 29 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9a0c3b7..5330e61 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4045,18 +4045,6 @@ void monitor_set_error(Monitor *mon, QError *qerror)
     }
 }
 
-static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
-{
-    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);
-    }
-}
-
 static void handle_user_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
@@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
     return input_dict;
 }
 
-static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
-                         const QDict *params)
-{
-    int ret;
-    QObject *data = NULL;
-
-    ret = cmd->mhandler.cmd_new(mon, params, &data);
-    handler_audit(mon, cmd, ret);
-    monitor_protocol_emitter(mon, data);
-    qobject_decref(data);
-}
-
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
     int err;
-    QObject *obj;
+    QObject *obj, *data;
     QDict *input, *args;
     const mon_cmd_t *cmd;
     const char *cmd_name;
     Monitor *mon = cur_mon;
 
     args = input = NULL;
+    data = NULL;
 
     obj = json_parser_parse(tokens, NULL);
     if (!obj) {
@@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    qmp_call_cmd(mon, cmd, args);
-    goto out;
+    if (cmd->mhandler.cmd_new(mon, args, &data)) {
+        /* Command failed... */
+        if (!monitor_has_error(mon)) {
+            /* ... without setting an error, so make one up */
+            qerror_report(QERR_UNDEFINED_ERROR);
+        }
+    }
 
 err_out:
-    monitor_protocol_emitter(mon, NULL);
-out:
+    monitor_protocol_emitter(mon, data);
+    qobject_decref(data);
     QDECREF(input);
     QDECREF(args);
 }
-- 
1.9.3

[Qemu-devel] [PATCH 03/20] monitor: Improve and document client_migrate_info protocol error

[Markus Armbruster]

Protocol must be spice, vnc isn't implemented.  Fix up documentation.

Attempts to use vnc or any other unknown protocol yield the misleading
error message "Invalid parameter 'protocol'".  Improve it to
"Parameter 'protocol' expects spice".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hmp-commands.hx | 1 +
 monitor.c       | 2 +-
 qmp-commands.hx | 3 ++-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index e864a6c..a8be73a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1022,6 +1022,7 @@ STEXI
 Set the spice/vnc connection info for the migration target.  The spice/vnc
 server will ask the spice/vnc client to automatically reconnect using the
 new parameters (if specified) once the vm migration finished successfully.
+Not yet implemented for VNC.
 ETEXI
 
     {
diff --git a/monitor.c b/monitor.c
index 5330e61..b507ee3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1063,7 +1063,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
         return 0;
     }
 
-    qerror_report(QERR_INVALID_PARAMETER, "protocol");
+    qerror_report(QERR_INVALID_PARAMETER_VALUE, "protocol", "spice");
     return -1;
 }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 14e109e..c267c89 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -795,10 +795,11 @@ client_migrate_info
 Set the spice/vnc connection info for the migration target.  The spice/vnc
 server will ask the spice/vnc client to automatically reconnect using the
 new parameters (if specified) once the vm migration finished successfully.
+Not yet implemented for VNC.
 
 Arguments:
 
-- "protocol":     protocol: "spice" or "vnc" (json-string)
+- "protocol":     must be "spice" (json-string)
 - "hostname":     migration target hostname (json-string)
 - "port":         spice/vnc tcp port for plaintext channels (json-int, optional)
 - "tls-port":     spice tcp port for tls-secured channels (json-int, optional)
-- 
1.9.3

[Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hmp-commands.hx  |  3 +--
 hmp.c            | 17 +++++++++++++++++
 hmp.h            |  1 +
 monitor.c        | 42 ++++++++++++++++++------------------------
 qapi-schema.json | 20 ++++++++++++++++++++
 qmp-commands.hx  |  2 +-
 6 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index a8be73a..480b8b9 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1012,8 +1012,7 @@ ETEXI
         .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
         .params     = "protocol hostname port tls-port cert-subject",
         .help       = "send migration info to spice/vnc client",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = client_migrate_info,
+        .mhandler.cmd = hmp_client_migrate_info,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index e17852d..5a43f9d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1250,6 +1250,23 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_client_migrate_info(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    const char *protocol = qdict_get_str(qdict, "protocol");
+    const char *hostname = qdict_get_str(qdict, "hostname");
+    bool has_port        = qdict_haskey(qdict, "port");
+    int port             = qdict_get_try_int(qdict, "port", -1);
+    bool has_tls_port    = qdict_haskey(qdict, "tls-port");
+    int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
+    const char *cert_subject = qdict_get_try_str(qdict, "cert-subject");
+
+    qmp_client_migrate_info(protocol, hostname,
+                            has_port, port, has_tls_port, tls_port,
+                            !!cert_subject, cert_subject, &err);
+    hmp_handle_error(mon, &err);
+}
+
 void hmp_set_password(Monitor *mon, const QDict *qdict)
 {
     const char *protocol  = qdict_get_str(qdict, "protocol");
diff --git a/hmp.h b/hmp.h
index a158e3f..b81439c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -67,6 +67,7 @@ void hmp_migrate_set_speed(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict);
+void hmp_client_migrate_info(Monitor *mon, const QDict *qdict);
 void hmp_set_password(Monitor *mon, const QDict *qdict);
 void hmp_expire_password(Monitor *mon, const QDict *qdict);
 void hmp_eject(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index b507ee3..4c37203 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1032,39 +1032,33 @@ static void hmp_info_trace_events(Monitor *mon, const QDict *qdict)
     qapi_free_TraceEventInfoList(events);
 }
 
-static int client_migrate_info(Monitor *mon, const QDict *qdict,
-                               QObject **ret_data)
+void qmp_client_migrate_info(const char *protocol, const char *hostname,
+                             bool has_port, int64_t port,
+                             bool has_tls_port, int64_t tls_port,
+                             bool has_cert_subject, const char *cert_subject,
+                             Error **errp)
 {
-    const char *protocol = qdict_get_str(qdict, "protocol");
-    const char *hostname = qdict_get_str(qdict, "hostname");
-    const char *subject  = qdict_get_try_str(qdict, "cert-subject");
-    int port             = qdict_get_try_int(qdict, "port", -1);
-    int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
-    Error *err = NULL;
-    int ret;
-
     if (strcmp(protocol, "spice") == 0) {
-        if (!qemu_using_spice(&err)) {
-            qerror_report_err(err);
-            error_free(err);
-            return -1;
+        if (!qemu_using_spice(errp)) {
+            return;
         }
 
-        if (port == -1 && tls_port == -1) {
-            qerror_report(QERR_MISSING_PARAMETER, "port/tls-port");
-            return -1;
+        if (!has_port && !has_tls_port) {
+            error_set(errp, QERR_MISSING_PARAMETER, "port/tls-port");
+            return;
         }
 
-        ret = qemu_spice_migrate_info(hostname, port, tls_port, subject);
-        if (ret != 0) {
-            qerror_report(QERR_UNDEFINED_ERROR);
-            return -1;
+        if (qemu_spice_migrate_info(hostname,
+                                    has_port ? port : -1,
+                                    has_tls_port ? tls_port : -1,
+                                    cert_subject)) {
+            error_set(errp, QERR_UNDEFINED_ERROR);
+            return;
         }
-        return 0;
+        return;
     }
 
-    qerror_report(QERR_INVALID_PARAMETER_VALUE, "protocol", "spice");
-    return -1;
+    error_set(errp, QERR_INVALID_PARAMETER_VALUE, "protocol", "spice");
 }
 
 static void hmp_logfile(Monitor *mon, const QDict *qdict)
diff --git a/qapi-schema.json b/qapi-schema.json
index f97ffa1..b8bac9c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -638,6 +638,26 @@
   'returns': 'MigrationParameters' }
 
 ##
+# @client_migrate_info
+#
+# Set the spice/vnc connection info for the migration target.  The
+# spice/vnc server will ask the spice/vnc client to automatically
+# reconnect using the new parameters (if specified) once the vm
+# migration finished successfully.  Not yet implemented for VNC.
+#
+# @protocol:     must be "spice"
+# @hostname:     migration target hostname
+# @port:         #optional spice/vnc tcp port for plaintext channels
+# @tls-port:     #optional spice tcp port for tls-secured channels
+# @cert-subject: #optional server certificate subject
+#
+# Since: 0.14.0
+##
+{ 'command': 'client_migrate_info',
+  'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int',
+            '*tls-port': 'int', '*cert-subject': 'str' } }
+
+##
 # @MouseInfo:
 #
 # Information about a mouse device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c267c89..4611b6b 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -785,7 +785,7 @@ EQMP
         .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
         .params     = "protocol hostname port tls-port cert-subject",
         .help       = "send migration info to spice/vnc client",
-        .mhandler.cmd_new = client_migrate_info,
+        .mhandler.cmd_new = qmp_marshal_input_client_migrate_info,
     },
 
 SQMP
-- 
1.9.3

[Qemu-devel] [PATCH 05/20] monitor: Use traditional command interface for HMP drive_del

[Markus Armbruster]

All QMP commands use the "new" handler interface (mhandler.cmd_new).
Most HMP commands still use the traditional interface (mhandler.cmd),
but a few use the "new" one.  Complicates handle_user_command() for no
gain, so I'm converting these to the traditional interface.

For drive_del, that's easy: hmp_drive_del() sheds its unused last
parameter, and its return value, which the caller ignored anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 blockdev.c                | 9 ++++-----
 hmp-commands.hx           | 3 +--
 include/sysemu/blockdev.h | 2 +-
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5eaf77e..de94a8b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2113,7 +2113,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
     aio_context_release(aio_context);
 }
 
-int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void hmp_drive_del(Monitor *mon, const QDict *qdict)
 {
     const char *id = qdict_get_str(qdict, "id");
     BlockBackend *blk;
@@ -2124,14 +2124,14 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     blk = blk_by_name(id);
     if (!blk) {
         error_report("Device '%s' not found", id);
-        return -1;
+        return;
     }
     bs = blk_bs(blk);
 
     if (!blk_legacy_dinfo(blk)) {
         error_report("Deleting device added with blockdev-add"
                      " is not supported");
-        return -1;
+        return;
     }
 
     aio_context = bdrv_get_aio_context(bs);
@@ -2140,7 +2140,7 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) {
         error_report_err(local_err);
         aio_context_release(aio_context);
-        return -1;
+        return;
     }
 
     /* quiesce block driver; prevent further io */
@@ -2163,7 +2163,6 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
 
     aio_context_release(aio_context);
-    return 0;
 }
 
 void qmp_block_resize(bool has_device, const char *device,
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 480b8b9..9939656 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -178,8 +178,7 @@ ETEXI
         .args_type  = "id:B",
         .params     = "device",
         .help       = "remove host block device",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = hmp_drive_del,
+        .mhandler.cmd = hmp_drive_del,
     },
 
 STEXI
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 7ca59b5..3104150 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -66,5 +66,5 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
 void qmp_change_blockdev(const char *device, const char *filename,
                          const char *format, Error **errp);
 void hmp_commit(Monitor *mon, const QDict *qdict);
-int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+void hmp_drive_del(Monitor *mon, const QDict *qdict);
 #endif
-- 
1.9.3

[Qemu-devel] [PATCH 06/20] monitor: Use traditional command interface for HMP device_add

[Markus Armbruster]

All QMP commands use the "new" handler interface (mhandler.cmd_new).
Most HMP commands still use the traditional interface (mhandler.cmd),
but a few use the "new" one.  Complicates handle_user_command() for no
gain, so I'm converting these to the traditional interface.

For device_add, that's easy: just wrap the obvious hmp_device_add()
around do_device_add().

monitor_user_noop() is now unused, drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hmp-commands.hx | 3 +--
 hmp.c           | 6 ++++++
 hmp.h           | 1 +
 monitor.c       | 2 --
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9939656..7776f16 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -653,8 +653,7 @@ ETEXI
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_device_add,
+        .mhandler.cmd = hmp_device_add,
         .command_completion = device_add_completion,
     },
 
diff --git a/hmp.c b/hmp.c
index 5a43f9d..514f22f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -22,6 +22,7 @@
 #include "qmp-commands.h"
 #include "qemu/sockets.h"
 #include "monitor/monitor.h"
+#include "monitor/qdev.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/string-output-visitor.h"
 #include "qapi-visit.h"
@@ -1499,6 +1500,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_device_add(Monitor *mon, const QDict *qdict)
+{
+    do_device_add(mon, qdict, NULL);
+}
+
 void hmp_device_del(Monitor *mon, const QDict *qdict)
 {
     const char *id = qdict_get_str(qdict, "id");
diff --git a/hmp.h b/hmp.h
index b81439c..a70ac4f 100644
--- a/hmp.h
+++ b/hmp.h
@@ -80,6 +80,7 @@ void hmp_block_job_pause(Monitor *mon, const QDict *qdict);
 void hmp_block_job_resume(Monitor *mon, const QDict *qdict);
 void hmp_block_job_complete(Monitor *mon, const QDict *qdict);
 void hmp_migrate(Monitor *mon, const QDict *qdict);
+void hmp_device_add(Monitor *mon, const QDict *qdict);
 void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 4c37203..64e7bd3 100644
--- a/monitor.c
+++ b/monitor.c
@@ -378,8 +378,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,
     return 0;
 }
 
-static void monitor_user_noop(Monitor *mon, const QObject *data) { }
-
 static inline int handler_is_qobject(const mon_cmd_t *cmd)
 {
     return cmd->user_print != NULL;
-- 
1.9.3

[Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error

[Markus Armbruster]

All QMP commands use the "new" handler interface (mhandler.cmd_new).
Most HMP commands still use the traditional interface (mhandler.cmd),
but a few use the "new" one.  Complicates handle_user_command() for no
gain, so I'm converting these to the traditional interface.

pcie_aer_inject_error's implementation is split into the
hmp_pcie_aer_inject_error() and pcie_aer_inject_error_print().  The
former is a peculiar crossbreed between HMP and QMP handler.  On
success, it works like a QMP handler: store QDict through ret_data
parameter, return 0.  Printing the QDict is left to
pcie_aer_inject_error_print().  On failure, it works more like an HMP
handler: print error to monitor, return negative number.

To convert to the traditional interface, turn
pcie_aer_inject_error_print() into a command handler wrapping around
hmp_pcie_aer_inject_error().  By convention, this command handler
should be called hmp_pcie_aer_inject_error(), so rename the existing
hmp_pcie_aer_inject_error() to do_pcie_aer_inject_error().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hmp-commands.hx         |  3 +--
 hw/pci/pci-stub.c       | 14 +-------------
 hw/pci/pcie_aer.c       | 39 ++++++++++++++++++++++-----------------
 include/sysemu/sysemu.h |  4 +---
 4 files changed, 25 insertions(+), 35 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 7776f16..0084114 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1184,8 +1184,7 @@ ETEXI
                       "<error_status> = error string or 32bit\n\t\t\t"
                       "<tlb header> = 32bit x 4\n\t\t\t"
                       "<tlb header prefix> = 32bit x 4",
-        .user_print  = pcie_aer_inject_error_print,
-        .mhandler.cmd_new = hmp_pcie_aer_inject_error,
+        .mhandler.cmd = hmp_pcie_aer_inject_error,
     },
 
 STEXI
diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c
index 5e564c3..f8f237e 100644
--- a/hw/pci/pci-stub.c
+++ b/hw/pci/pci-stub.c
@@ -29,19 +29,7 @@ PciInfoList *qmp_query_pci(Error **errp)
     return NULL;
 }
 
-static void pci_error_message(Monitor *mon)
+void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
 {
     monitor_printf(mon, "PCI devices not supported\n");
 }
-
-int hmp_pcie_aer_inject_error(Monitor *mon,
-                             const QDict *qdict, QObject **ret_data)
-{
-    pci_error_message(mon);
-    return -ENOSYS;
-}
-
-void pcie_aer_inject_error_print(Monitor *mon, const QObject *data)
-{
-    pci_error_message(mon);
-}
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index b48c09c..c8dea8e 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -815,21 +815,6 @@ const VMStateDescription vmstate_pcie_aer_log = {
     }
 };
 
-void pcie_aer_inject_error_print(Monitor *mon, const QObject *data)
-{
-    QDict *qdict;
-    int devfn;
-    assert(qobject_type(data) == QTYPE_QDICT);
-    qdict = qobject_to_qdict(data);
-
-    devfn = (int)qdict_get_int(qdict, "devfn");
-    monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n",
-                   qdict_get_str(qdict, "id"),
-                   qdict_get_str(qdict, "root_bus"),
-                   (int) qdict_get_int(qdict, "bus"),
-                   PCI_SLOT(devfn), PCI_FUNC(devfn));
-}
-
 typedef struct PCIEAERErrorName {
     const char *name;
     uint32_t val;
@@ -962,8 +947,8 @@ static int pcie_aer_parse_error_string(const char *error_name,
     return -EINVAL;
 }
 
-int hmp_pcie_aer_inject_error(Monitor *mon,
-                             const QDict *qdict, QObject **ret_data)
+static int do_pcie_aer_inject_error(Monitor *mon,
+                                    const QDict *qdict, QObject **ret_data)
 {
     const char *id = qdict_get_str(qdict, "id");
     const char *error_name;
@@ -1035,3 +1020,23 @@ int hmp_pcie_aer_inject_error(Monitor *mon,
 
     return 0;
 }
+
+void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict)
+{
+    QObject *data;
+    int devfn;
+
+    if (do_pcie_aer_inject_error(mon, qdict, &data) < 0) {
+        return;
+    }
+
+    assert(qobject_type(data) == QTYPE_QDICT);
+    qdict = qobject_to_qdict(data);
+
+    devfn = (int)qdict_get_int(qdict, "devfn");
+    monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n",
+                   qdict_get_str(qdict, "id"),
+                   qdict_get_str(qdict, "root_bus"),
+                   (int) qdict_get_int(qdict, "bus"),
+                   PCI_SLOT(devfn), PCI_FUNC(devfn));
+}
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8a52934..e10c2c5 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -161,9 +161,7 @@ extern unsigned int nb_prom_envs;
 void hmp_drive_add(Monitor *mon, const QDict *qdict);
 
 /* pcie aer error injection */
-void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
-int hmp_pcie_aer_inject_error(Monitor *mon,
-                             const QDict *qdict, QObject **ret_data);
+void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict);
 
 /* serial ports */
 
-- 
1.9.3

[Qemu-devel] [PATCH 08/20] monitor: Drop unused "new" HMP command interface

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/monitor.c b/monitor.c
index 64e7bd3..3a7e625 100644
--- a/monitor.c
+++ b/monitor.c
@@ -123,7 +123,6 @@ typedef struct mon_cmd_t {
     const char *args_type;
     const char *params;
     const char *help;
-    void (*user_print)(Monitor *mon, const QObject *data);
     union {
         void (*cmd)(Monitor *mon, const QDict *qdict);
         int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
@@ -378,11 +377,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,
     return 0;
 }
 
-static inline int handler_is_qobject(const mon_cmd_t *cmd)
-{
-    return cmd->user_print != NULL;
-}
-
 static inline int monitor_has_error(const Monitor *mon)
 {
     return mon->error != NULL;
@@ -4045,24 +4039,10 @@ static void handle_user_command(Monitor *mon, const char *cmdline)
     qdict = qdict_new();
 
     cmd = monitor_parse_command(mon, cmdline, 0, mon->cmd_table, qdict);
-    if (!cmd)
-        goto out;
-
-    if (handler_is_qobject(cmd)) {
-        QObject *data = NULL;
-
-        /* XXX: ignores the error code */
-        cmd->mhandler.cmd_new(mon, qdict, &data);
-        assert(!monitor_has_error(mon));
-        if (data) {
-            cmd->user_print(mon, data);
-            qobject_decref(data);
-        }
-    } else {
+    if (cmd) {
         cmd->mhandler.cmd(mon, qdict);
     }
 
-out:
     QDECREF(qdict);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 09/20] monitor: Propagate errors through qmp_check_client_args()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 65 ++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 33 insertions(+), 32 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3a7e625..01b1a5f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4736,8 +4736,9 @@ static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
  *               the QMP_ACCEPT_UNKNOWNS flag is set, then the
  *               checking is skipped for it.
  */
-static int check_client_args_type(const QDict *client_args,
-                                  const QDict *cmd_args, int flags)
+static void check_client_args_type(const QDict *client_args,
+                                   const QDict *cmd_args, int flags,
+                                   Error **errp)
 {
     const QDictEntry *ent;
 
@@ -4754,8 +4755,8 @@ static int check_client_args_type(const QDict *client_args,
                 continue;
             }
             /* client arg doesn't exist */
-            qerror_report(QERR_INVALID_PARAMETER, client_arg_name);
-            return -1;
+            error_set(errp, QERR_INVALID_PARAMETER, client_arg_name);
+            return;
         }
 
         arg_type = qobject_to_qstring(obj);
@@ -4767,9 +4768,9 @@ static int check_client_args_type(const QDict *client_args,
         case 'B':
         case 's':
             if (qobject_type(client_arg) != QTYPE_QSTRING) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
-                              "string");
-                return -1;
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE,
+                          client_arg_name, "string");
+                return;
             }
         break;
         case 'i':
@@ -4777,25 +4778,25 @@ static int check_client_args_type(const QDict *client_args,
         case 'M':
         case 'o':
             if (qobject_type(client_arg) != QTYPE_QINT) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
-                              "int");
-                return -1; 
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE,
+                          client_arg_name, "int");
+                return;
             }
             break;
         case 'T':
             if (qobject_type(client_arg) != QTYPE_QINT &&
                 qobject_type(client_arg) != QTYPE_QFLOAT) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
-                              "number");
-               return -1; 
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE,
+                          client_arg_name, "number");
+                return;
             }
             break;
         case 'b':
         case '-':
             if (qobject_type(client_arg) != QTYPE_QBOOL) {
-                qerror_report(QERR_INVALID_PARAMETER_TYPE, client_arg_name,
-                              "bool");
-               return -1; 
+                error_set(errp, QERR_INVALID_PARAMETER_TYPE,
+                          client_arg_name, "bool");
+                return;
             }
             break;
         case 'O':
@@ -4814,16 +4815,15 @@ static int check_client_args_type(const QDict *client_args,
             abort();
         }
     }
-
-    return 0;
 }
 
 /*
  * - Check if the client has passed all mandatory args
  * - Set special flags for argument validation
  */
-static int check_mandatory_args(const QDict *cmd_args,
-                                const QDict *client_args, int *flags)
+static void check_mandatory_args(const QDict *cmd_args,
+                                 const QDict *client_args, int *flags,
+                                 Error **errp)
 {
     const QDictEntry *ent;
 
@@ -4838,12 +4838,10 @@ static int check_mandatory_args(const QDict *cmd_args,
         } else if (qstring_get_str(type)[0] != '-' &&
                    qstring_get_str(type)[1] != '?' &&
                    !qdict_haskey(client_args, cmd_arg_name)) {
-            qerror_report(QERR_MISSING_PARAMETER, cmd_arg_name);
-            return -1;
+            error_set(errp, QERR_MISSING_PARAMETER, cmd_arg_name);
+            return;
         }
     }
-
-    return 0;
 }
 
 static QDict *qdict_from_args_type(const char *args_type)
@@ -4899,24 +4897,26 @@ out:
  * 3. Each argument provided by the client must have the type expected
  *    by the command
  */
-static int qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args)
+static void qmp_check_client_args(const mon_cmd_t *cmd, QDict *client_args,
+                                  Error **errp)
 {
-    int flags, err;
+    Error *err = NULL;
+    int flags;
     QDict *cmd_args;
 
     cmd_args = qdict_from_args_type(cmd->args_type);
 
     flags = 0;
-    err = check_mandatory_args(cmd_args, client_args, &flags);
+    check_mandatory_args(cmd_args, client_args, &flags, &err);
     if (err) {
         goto out;
     }
 
-    err = check_client_args_type(client_args, cmd_args, flags);
+    check_client_args_type(client_args, cmd_args, flags, &err);
 
 out:
+    error_propagate(errp, err);
     QDECREF(cmd_args);
-    return err;
 }
 
 /*
@@ -4977,7 +4977,7 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
 
 static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 {
-    int err;
+    Error *local_err = NULL;
     QObject *obj, *data;
     QDict *input, *args;
     const mon_cmd_t *cmd;
@@ -5023,8 +5023,9 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         QINCREF(args);
     }
 
-    err = qmp_check_client_args(cmd, args);
-    if (err < 0) {
+    qmp_check_client_args(cmd, args, &local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
         goto err_out;
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH 10/20] monitor: Propagate errors through qmp_check_input_obj()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/monitor.c b/monitor.c
index 01b1a5f..1d7ad0a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4929,14 +4929,14 @@ out:
  * 5. If the "id" key exists, it can be anything (ie. json-value)
  * 6. Any argument not listed above is considered invalid
  */
-static QDict *qmp_check_input_obj(QObject *input_obj)
+static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp)
 {
     const QDictEntry *ent;
     int has_exec_key = 0;
     QDict *input_dict;
 
     if (qobject_type(input_obj) != QTYPE_QDICT) {
-        qerror_report(QERR_QMP_BAD_INPUT_OBJECT, "object");
+        error_set(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
         return NULL;
     }
 
@@ -4948,27 +4948,27 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
 
         if (!strcmp(arg_name, "execute")) {
             if (qobject_type(arg_obj) != QTYPE_QSTRING) {
-                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
-                              "string");
+                error_set(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+                          "execute", "string");
                 return NULL;
             }
             has_exec_key = 1;
         } else if (!strcmp(arg_name, "arguments")) {
             if (qobject_type(arg_obj) != QTYPE_QDICT) {
-                qerror_report(QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "arguments",
-                              "object");
+                error_set(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+                          "arguments", "object");
                 return NULL;
             }
         } else if (!strcmp(arg_name, "id")) {
             /* FIXME: check duplicated IDs for async commands */
         } else {
-            qerror_report(QERR_QMP_EXTRA_MEMBER, arg_name);
+            error_set(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
             return NULL;
         }
     }
 
     if (!has_exec_key) {
-        qerror_report(QERR_QMP_BAD_INPUT_OBJECT, "execute");
+        error_set(errp, QERR_QMP_BAD_INPUT_OBJECT, "execute");
         return NULL;
     }
 
@@ -4994,8 +4994,9 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    input = qmp_check_input_obj(obj);
+    input = qmp_check_input_obj(obj, &local_err);
     if (!input) {
+        qerror_report_err(local_err);
         qobject_decref(obj);
         goto err_out;
     }
-- 
1.9.3

[Qemu-devel] [PATCH 11/20] monitor: Wean monitor_protocol_emitter() off mon->error

[Markus Armbruster]

Move mon->error handling to its caller handle_qmp_command().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 1d7ad0a..c732203 100644
--- a/monitor.c
+++ b/monitor.c
@@ -407,13 +407,14 @@ static QDict *build_qmp_error_dict(const QError *err)
     return qobject_to_qdict(obj);
 }
 
-static void monitor_protocol_emitter(Monitor *mon, QObject *data)
+static void monitor_protocol_emitter(Monitor *mon, QObject *data,
+                                     QError *err)
 {
     QDict *qmp;
 
     trace_monitor_protocol_emitter(mon);
 
-    if (!monitor_has_error(mon)) {
+    if (!err) {
         /* success response */
         qmp = qdict_new();
         if (data) {
@@ -425,9 +426,7 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data)
         }
     } else {
         /* error response */
-        qmp = build_qmp_error_dict(mon->error);
-        QDECREF(mon->error);
-        mon->error = NULL;
+        qmp = build_qmp_error_dict(err);
     }
 
     if (mon->mc->id) {
@@ -5039,8 +5038,10 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     }
 
 err_out:
-    monitor_protocol_emitter(mon, data);
+    monitor_protocol_emitter(mon, data, mon->error);
     qobject_decref(data);
+    QDECREF(mon->error);
+    mon->error = NULL;
     QDECREF(input);
     QDECREF(args);
 }
-- 
1.9.3

[Qemu-devel] [PATCH 12/20] monitor: Inline monitor_has_error() into its only caller

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index c732203..71ca03f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -377,11 +377,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,
     return 0;
 }
 
-static inline int monitor_has_error(const Monitor *mon)
-{
-    return mon->error != NULL;
-}
-
 static void monitor_json_emitter(Monitor *mon, const QObject *data)
 {
     QString *json;
@@ -5031,7 +5026,7 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 
     if (cmd->mhandler.cmd_new(mon, args, &data)) {
         /* Command failed... */
-        if (!monitor_has_error(mon)) {
+        if (!mon->error) {
             /* ... without setting an error, so make one up */
             qerror_report(QERR_UNDEFINED_ERROR);
         }
-- 
1.9.3

[Qemu-devel] [PATCH 13/20] monitor: Limit QError use to command handlers

[Markus Armbruster]

The previous commits narrowed use of QError to handle_qmp_command()
and its helpers monitor_protocol_emitter(), build_qmp_error_dict().
Narrow it further to just the command handler call: instead of
converting Error to QError throughout handle_qmp_command(), convert
the QError gotten from the command handler to Error, and switch the
helpers from QError to Error.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index 71ca03f..65ef400 100644
--- a/monitor.c
+++ b/monitor.c
@@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
     QDECREF(json);
 }
 
-static QDict *build_qmp_error_dict(const QError *err)
+static QDict *build_qmp_error_dict(Error *err)
 {
     QObject *obj;
 
-    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
-                             ErrorClass_lookup[err->err_class],
-                             qerror_human(err));
+    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
+                             ErrorClass_lookup[error_get_class(err)],
+                             error_get_pretty(err));
 
     return qobject_to_qdict(obj);
 }
 
 static void monitor_protocol_emitter(Monitor *mon, QObject *data,
-                                     QError *err)
+                                     Error *err)
 {
     QDict *qmp;
 
@@ -4984,13 +4984,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     obj = json_parser_parse(tokens, NULL);
     if (!obj) {
         // FIXME: should be triggered in json_parser_parse()
-        qerror_report(QERR_JSON_PARSING);
+        error_set(&local_err, QERR_JSON_PARSING);
         goto err_out;
     }
 
     input = qmp_check_input_obj(obj, &local_err);
     if (!input) {
-        qerror_report_err(local_err);
         qobject_decref(obj);
         goto err_out;
     }
@@ -5002,8 +5001,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
     trace_handle_qmp_command(mon, cmd_name);
     cmd = qmp_find_cmd(cmd_name);
     if (!cmd) {
-        qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
-                      "The command %s has not been found", cmd_name);
+        error_set(&local_err, ERROR_CLASS_COMMAND_NOT_FOUND,
+                  "The command %s has not been found", cmd_name);
         goto err_out;
     }
     if (invalid_qmp_mode(mon, cmd)) {
@@ -5020,7 +5019,6 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
 
     qmp_check_client_args(cmd, args, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
         goto err_out;
     }
 
@@ -5028,12 +5026,16 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         /* Command failed... */
         if (!mon->error) {
             /* ... without setting an error, so make one up */
-            qerror_report(QERR_UNDEFINED_ERROR);
+            error_set(&local_err, QERR_UNDEFINED_ERROR);
         }
     }
+    if (mon->error) {
+        error_set(&local_err, mon->error->err_class, "%s",
+                  mon->error->err_msg);
+    }
 
 err_out:
-    monitor_protocol_emitter(mon, data, mon->error);
+    monitor_protocol_emitter(mon, data, local_err);
     qobject_decref(data);
     QDECREF(mon->error);
     mon->error = NULL;
-- 
1.9.3

[Qemu-devel] [PATCH 14/20] monitor: Rename handle_user_command() to handle_hmp_command()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index 65ef400..ecadda7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -574,7 +574,7 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
     return 0;
 }
 
-static void handle_user_command(Monitor *mon, const char *cmdline);
+static void handle_hmp_command(Monitor *mon, const char *cmdline);
 
 static void monitor_data_init(Monitor *mon)
 {
@@ -613,7 +613,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
         }
     }
 
-    handle_user_command(&hmp, command_line);
+    handle_hmp_command(&hmp, command_line);
     cur_mon = old_mon;
 
     qemu_mutex_lock(&hmp.out_lock);
@@ -4025,7 +4025,7 @@ void monitor_set_error(Monitor *mon, QError *qerror)
     }
 }
 
-static void handle_user_command(Monitor *mon, const char *cmdline)
+static void handle_hmp_command(Monitor *mon, const char *cmdline)
 {
     QDict *qdict;
     const mon_cmd_t *cmd;
@@ -5071,7 +5071,7 @@ static void monitor_read(void *opaque, const uint8_t *buf, int size)
         if (size == 0 || buf[size - 1] != 0)
             monitor_printf(cur_mon, "corrupted command\n");
         else
-            handle_user_command(cur_mon, (char *)buf);
+            handle_hmp_command(cur_mon, (char *)buf);
     }
 
     cur_mon = old_mon;
@@ -5083,7 +5083,7 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
     Monitor *mon = opaque;
 
     monitor_suspend(mon);
-    handle_user_command(mon, cmdline);
+    handle_hmp_command(mon, cmdline);
     monitor_resume(mon);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 15/20] monitor: Rename monitor_control_read(), monitor_control_event()

[Markus Armbruster]

... to monitor_qmp_read(), monitor_qmp_event().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index ecadda7..977c0fd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5043,10 +5043,7 @@ err_out:
     QDECREF(args);
 }
 
-/**
- * monitor_control_read(): Read and handle QMP input
- */
-static void monitor_control_read(void *opaque, const uint8_t *buf, int size)
+static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
     Monitor *old_mon = cur_mon;
 
@@ -5111,10 +5108,7 @@ static QObject *get_qmp_greeting(void)
     return qobject_from_jsonf("{'QMP':{'version': %p,'capabilities': []}}",ver);
 }
 
-/**
- * monitor_control_event(): Print QMP gretting
- */
-static void monitor_control_event(void *opaque, int event)
+static void monitor_qmp_event(void *opaque, int event)
 {
     QObject *data;
     Monitor *mon = opaque;
@@ -5264,8 +5258,8 @@ void monitor_init(CharDriverState *chr, int flags)
     if (monitor_ctrl_mode(mon)) {
         mon->mc = g_malloc0(sizeof(MonitorControl));
         /* Control mode requires special handlers */
-        qemu_chr_add_handlers(chr, monitor_can_read, monitor_control_read,
-                              monitor_control_event, mon);
+        qemu_chr_add_handlers(chr, monitor_can_read, monitor_qmp_read,
+                              monitor_qmp_event, mon);
         qemu_chr_fe_set_echo(chr, true);
 
         json_message_parser_init(&mon->mc->parser, handle_qmp_command);
-- 
1.9.3

[Qemu-devel] [PATCH 16/20] monitor: Unbox Monitor member mc and rename to qmp

[Markus Armbruster]

While there, rename its type as well, from MonitorControl to
MonitorQMP.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/monitor.c b/monitor.c
index 977c0fd..4b397e6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -161,11 +161,11 @@ struct MonFdset {
     QLIST_ENTRY(MonFdset) next;
 };
 
-typedef struct MonitorControl {
+typedef struct {
     QObject *id;
     JSONMessageParser parser;
     int command_mode;
-} MonitorControl;
+} MonitorQMP;
 
 /*
  * To prevent flooding clients, events can be throttled. The
@@ -195,7 +195,7 @@ struct Monitor {
     int mux_out;
 
     ReadLineState *rs;
-    MonitorControl *mc;
+    MonitorQMP qmp;
     CPUState *mon_cpu;
     BlockCompletionFunc *password_completion_cb;
     void *password_opaque;
@@ -228,7 +228,7 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
 
 static inline int qmp_cmd_mode(const Monitor *mon)
 {
-    return (mon->mc ? mon->mc->command_mode : 0);
+    return mon->qmp.command_mode;
 }
 
 /* Return true if in control mode, false otherwise */
@@ -424,9 +424,9 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data,
         qmp = build_qmp_error_dict(err);
     }
 
-    if (mon->mc->id) {
-        qdict_put_obj(qmp, "id", mon->mc->id);
-        mon->mc->id = NULL;
+    if (mon->qmp.id) {
+        qdict_put_obj(qmp, "id", mon->qmp.id);
+        mon->qmp.id = NULL;
     }
 
     monitor_json_emitter(mon, QOBJECT(qmp));
@@ -568,7 +568,7 @@ static int do_qmp_capabilities(Monitor *mon, const QDict *params,
 {
     /* Will setup QMP capabilities in the future */
     if (monitor_ctrl_mode(mon)) {
-        mon->mc->command_mode = 1;
+        mon->qmp.command_mode = 1;
     }
 
     return 0;
@@ -4994,8 +4994,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
         goto err_out;
     }
 
-    mon->mc->id = qdict_get(input, "id");
-    qobject_incref(mon->mc->id);
+    mon->qmp.id = qdict_get(input, "id");
+    qobject_incref(mon->qmp.id);
 
     cmd_name = qdict_get_str(input, "execute");
     trace_handle_qmp_command(mon, cmd_name);
@@ -5049,7 +5049,7 @@ static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 
     cur_mon = opaque;
 
-    json_message_parser_feed(&cur_mon->mc->parser, (const char *) buf, size);
+    json_message_parser_feed(&cur_mon->qmp.parser, (const char *) buf, size);
 
     cur_mon = old_mon;
 }
@@ -5115,15 +5115,15 @@ static void monitor_qmp_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_OPENED:
-        mon->mc->command_mode = 0;
+        mon->qmp.command_mode = 0;
         data = get_qmp_greeting();
         monitor_json_emitter(mon, data);
         qobject_decref(data);
         mon_refcount++;
         break;
     case CHR_EVENT_CLOSED:
-        json_message_parser_destroy(&mon->mc->parser);
-        json_message_parser_init(&mon->mc->parser, handle_qmp_command);
+        json_message_parser_destroy(&mon->qmp.parser);
+        json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
         mon_refcount--;
         monitor_fdsets_cleanup();
         break;
@@ -5255,14 +5255,11 @@ void monitor_init(CharDriverState *chr, int flags)
         monitor_read_command(mon, 0);
     }
 
-    if (monitor_ctrl_mode(mon)) {
-        mon->mc = g_malloc0(sizeof(MonitorControl));
-        /* Control mode requires special handlers */
+    if (flags & MONITOR_USE_CONTROL) {
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_qmp_read,
                               monitor_qmp_event, mon);
         qemu_chr_fe_set_echo(chr, true);
-
-        json_message_parser_init(&mon->mc->parser, handle_qmp_command);
+        json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
     } else {
         qemu_chr_add_handlers(chr, monitor_can_read, monitor_read,
                               monitor_event, mon);
-- 
1.9.3

[Qemu-devel] [PATCH 17/20] monitor: Drop do_qmp_capabilities()'s superfluous QMP check

[Markus Armbruster]

Superfluous since commit 30f5041 removed it from HMP.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4b397e6..6e9cc8c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -566,11 +566,7 @@ static void monitor_qapi_event_init(void)
 static int do_qmp_capabilities(Monitor *mon, const QDict *params,
                                QObject **ret_data)
 {
-    /* Will setup QMP capabilities in the future */
-    if (monitor_ctrl_mode(mon)) {
-        mon->qmp.command_mode = 1;
-    }
-
+    mon->qmp.command_mode = 1;
     return 0;
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 18/20] monitor: Turn int command_mode into bool in_command_mode

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/monitor.c b/monitor.c
index 6e9cc8c..f31a05d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -164,7 +164,12 @@ struct MonFdset {
 typedef struct {
     QObject *id;
     JSONMessageParser parser;
-    int command_mode;
+    /*
+     * When a client connects, we're in capabilities negotiation mode.
+     * When command qmp_capabilities succeeds, we go into command
+     * mode.
+     */
+    bool in_command_mode;       /* are we in command mode? */
 } MonitorQMP;
 
 /*
@@ -226,11 +231,6 @@ Monitor *default_mon;
 static void monitor_command_cb(void *opaque, const char *cmdline,
                                void *readline_opaque);
 
-static inline int qmp_cmd_mode(const Monitor *mon)
-{
-    return mon->qmp.command_mode;
-}
-
 /* Return true if in control mode, false otherwise */
 static inline int monitor_ctrl_mode(const Monitor *mon)
 {
@@ -446,7 +446,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
 
     trace_monitor_protocol_event_emit(event, data);
     QLIST_FOREACH(mon, &mon_list, entry) {
-        if (monitor_ctrl_mode(mon) && qmp_cmd_mode(mon)) {
+        if (monitor_ctrl_mode(mon) && mon->qmp.in_command_mode) {
             monitor_json_emitter(mon, data);
         }
     }
@@ -566,7 +566,7 @@ static void monitor_qapi_event_init(void)
 static int do_qmp_capabilities(Monitor *mon, const QDict *params,
                                QObject **ret_data)
 {
-    mon->qmp.command_mode = 1;
+    mon->qmp.in_command_mode = true;
     return 0;
 }
 
@@ -4701,13 +4701,14 @@ static int monitor_can_read(void *opaque)
 static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd)
 {
     bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
-    if (is_cap && qmp_cmd_mode(mon)) {
+
+    if (is_cap && mon->qmp.in_command_mode) {
         qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
                       "Capabilities negotiation is already complete, command "
                       "'%s' ignored", cmd->name);
         return true;
     }
-    if (!is_cap && !qmp_cmd_mode(mon)) {
+    if (!is_cap && !mon->qmp.in_command_mode) {
         qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND,
                       "Expecting capabilities negotiation with "
                       "'qmp_capabilities' before command '%s'", cmd->name);
@@ -5111,7 +5112,7 @@ static void monitor_qmp_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_OPENED:
-        mon->qmp.command_mode = 0;
+        mon->qmp.in_command_mode = false;
         data = get_qmp_greeting();
         monitor_json_emitter(mon, data);
         qobject_decref(data);
-- 
1.9.3

[Qemu-devel] [PATCH 19/20] monitor: Rename monitor_ctrl_mode() to monitor_is_qmp()

[Markus Armbruster]

... and change return type to bool.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index f31a05d..2bdde35 100644
--- a/monitor.c
+++ b/monitor.c
@@ -231,8 +231,10 @@ Monitor *default_mon;
 static void monitor_command_cb(void *opaque, const char *cmdline,
                                void *readline_opaque);
 
-/* Return true if in control mode, false otherwise */
-static inline int monitor_ctrl_mode(const Monitor *mon)
+/**
+ * Is @mon a QMP monitor?
+ */
+static inline bool monitor_is_qmp(const Monitor *mon)
 {
     return (mon->flags & MONITOR_USE_CONTROL);
 }
@@ -240,7 +242,7 @@ static inline int monitor_ctrl_mode(const Monitor *mon)
 /* Return non-zero iff we have a current monitor, and it is in QMP mode.  */
 int monitor_cur_is_qmp(void)
 {
-    return cur_mon && monitor_ctrl_mode(cur_mon);
+    return cur_mon && monitor_is_qmp(cur_mon);
 }
 
 void monitor_read_command(Monitor *mon, int show_prompt)
@@ -350,7 +352,7 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     if (!mon)
         return;
 
-    if (monitor_ctrl_mode(mon)) {
+    if (monitor_is_qmp(mon)) {
         return;
     }
 
@@ -446,7 +448,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
 
     trace_monitor_protocol_event_emit(event, data);
     QLIST_FOREACH(mon, &mon_list, entry) {
-        if (monitor_ctrl_mode(mon) && mon->qmp.in_command_mode) {
+        if (monitor_is_qmp(mon) && mon->qmp.in_command_mode) {
             monitor_json_emitter(mon, data);
         }
     }
-- 
1.9.3

[Qemu-devel] [PATCH 20/20] monitor: Change return type of monitor_cur_is_qmp() to bool

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/monitor/monitor.h | 2 +-
 monitor.c                 | 6 ++++--
 stubs/mon-is-qmp.c        | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index d409b6a..57f8394 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,7 +16,7 @@ extern Monitor *default_mon;
 #define MONITOR_USE_CONTROL   0x04
 #define MONITOR_USE_PRETTY    0x08
 
-int monitor_cur_is_qmp(void);
+bool monitor_cur_is_qmp(void);
 
 void monitor_init(CharDriverState *chr, int flags);
 
diff --git a/monitor.c b/monitor.c
index 2bdde35..6ab1817 100644
--- a/monitor.c
+++ b/monitor.c
@@ -239,8 +239,10 @@ static inline bool monitor_is_qmp(const Monitor *mon)
     return (mon->flags & MONITOR_USE_CONTROL);
 }
 
-/* Return non-zero iff we have a current monitor, and it is in QMP mode.  */
-int monitor_cur_is_qmp(void)
+/**
+ * Is the current monitor, if any, a QMP monitor?
+ */
+bool monitor_cur_is_qmp(void)
 {
     return cur_mon && monitor_is_qmp(cur_mon);
 }
diff --git a/stubs/mon-is-qmp.c b/stubs/mon-is-qmp.c
index 1f0a8fd..3cff163 100644
--- a/stubs/mon-is-qmp.c
+++ b/stubs/mon-is-qmp.c
@@ -1,7 +1,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-int monitor_cur_is_qmp(void)
+bool monitor_cur_is_qmp(void)
 {
     return 0;
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 01/20] monitor: Drop broken, unused asynchronous command interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> The asynchronous monitor command interface goes back to commit 940cc30
> (Jan 2010).  Added a third case to command execution.  The hope back
> then according to the commit message was that all commands get
> converted to the asynchronous interface, killing off the other two
> cases.  Didn't happen.
> 
> The initial asynchronous commands balloon and info balloon were
> converted back to synchronous long ago (commit 96637bc and d72f32),
> with commit messages calling the asynchronous interface "not fully
> working" and "deprecated".  The only other user went away in commit
> 3b5704b.

The history is helpful; thanks.

> 
> New code generally uses synchronous commands and asynchronous events.
> 
> What exactly is still "not fully working" with asynchronous commands?
> Well, here's a bug that defeats actual asynchronous use pretty
> reliably: the reply's ID is wrong (and has always been wrong) unless
> you use the command synchronously!  To reproduce, we need an
> asynchronous command, so we have to go back before the commit 3b5704b.
> Run QEMU with spice:
> 
>     $ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing -qmp stdio
>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major":
> 2}, "package": ""}, "capabilities": []}}
> 
> Connect a spice client in another terminal:
> 
>     $ remote-viewer spice://localhost:5900
> 
> Set up a migration destination dummy in a third terminal:
> 
>     $ socat TCP-LISTEN:12345 STDIO
> 
> Now paste the following into the QMP monitor:
> 
>     { "execute": "qmp_capabilities" }

If you stick and "id":"i0" here...

>     { "execute": "client_migrate_info", "id": "i1", "arguments": { "protocol": "spice", "hostname": "localhost", "port": 12345 } }
>     { "execute": "query-kvm", "id": "i2" }
> 
> Produces two replies immediately, one to qmp_capabilities, and one to
> query-kvm:
> 
>     {"return": {}}

...it should show up here, for even less ambiguity about the problems
with the async response.

>     {"return": {"enabled": false, "present": true}, "id": "i2"}
> 
> Both are correct.  Two lines of debug output from libspice-server not
> shown.
> 
> Now EOF socat's standard input to make it close the connection.  This
> makes the asynchronous client_migrate_info complete.  It replies:
> 
>     {"return": {}}
> 
> Bug: "id": "i1" is missing.  Two lines of debug output from
> libspice-server not shown.  Cherry on top: storage for the missing ID
> is leaked.
> 
> Get rid of this stuff before somebody hurts himself with it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/monitor/monitor.h |  5 ----
>  monitor.c                 | 68 ++---------------------------------------------
>  2 files changed, 2 insertions(+), 71 deletions(-)

Certainly takes an axe to it!

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 02/20] monitor: Clean up after previous commit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2723 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

Commit message is sparse; I would have mentioned [1] and [2].

>  monitor.c | 40 +++++++++++-----------------------------
>  1 file changed, 11 insertions(+), 29 deletions(-)
> 
>  
> -static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
> -{
> -    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);
> -    }
> -}

[2] - handler_audit() goes away now that it has no caller

> -
>  static void handle_user_command(Monitor *mon, const char *cmdline)
>  {
>      QDict *qdict;
> @@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
>      return input_dict;
>  }
>  
> -static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
> -                         const QDict *params)
> -{
> -    int ret;
> -    QObject *data = NULL;
> -
> -    ret = cmd->mhandler.cmd_new(mon, params, &data);
> -    handler_audit(mon, cmd, ret);
> -    monitor_protocol_emitter(mon, data);
> -    qobject_decref(data);
> -}

[1] - qmp_call_cmd() is inlined and simplified...

> -
>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>  {
>      int err;
> -    QObject *obj;
> +    QObject *obj, *data;
>      QDict *input, *args;
>      const mon_cmd_t *cmd;
>      const char *cmd_name;
>      Monitor *mon = cur_mon;
>  
>      args = input = NULL;
> +    data = NULL;
>  
>      obj = json_parser_parse(tokens, NULL);
>      if (!obj) {
> @@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>          goto err_out;
>      }
>  
> -    qmp_call_cmd(mon, cmd, args);
> -    goto out;
> +    if (cmd->mhandler.cmd_new(mon, args, &data)) {
> +        /* Command failed... */
> +        if (!monitor_has_error(mon)) {
> +            /* ... without setting an error, so make one up */
> +            qerror_report(QERR_UNDEFINED_ERROR);
> +        }
> +    }

...into its lone caller.

>  
>  err_out:
> -    monitor_protocol_emitter(mon, NULL);
> -out:
> +    monitor_protocol_emitter(mon, data);
> +    qobject_decref(data);
>      QDECREF(input);
>      QDECREF(args);
>  }
> 

But the code cleanup itself looks sane, so with the improved commit message:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 03/20] monitor: Improve and document client_migrate_info protocol error

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Protocol must be spice, vnc isn't implemented.  Fix up documentation.
> 
> Attempts to use vnc or any other unknown protocol yield the misleading
> error message "Invalid parameter 'protocol'".  Improve it to
> "Parameter 'protocol' expects spice".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hmp-commands.hx | 1 +
>  monitor.c       | 2 +-
>  qmp-commands.hx | 3 ++-
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hmp-commands.hx  |  3 +--
>  hmp.c            | 17 +++++++++++++++++
>  hmp.h            |  1 +
>  monitor.c        | 42 ++++++++++++++++++------------------------
>  qapi-schema.json | 20 ++++++++++++++++++++
>  qmp-commands.hx  |  2 +-
>  6 files changed, 58 insertions(+), 27 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -638,6 +638,26 @@
>    'returns': 'MigrationParameters' }
>  
>  ##
> +# @client_migrate_info

Should we name this new command 'client-migrate-info'?

/me goes and checks...

Oh, wow, the QMP command IS advertised by 'query-commands' in spite of
not having this wrapper, even in qemu 2.3.

> +#
> +# Set the spice/vnc connection info for the migration target.  The
> +# spice/vnc server will ask the spice/vnc client to automatically
> +# reconnect using the new parameters (if specified) once the vm
> +# migration finished successfully.  Not yet implemented for VNC.
> +#
> +# @protocol:     must be "spice"
> +# @hostname:     migration target hostname
> +# @port:         #optional spice/vnc tcp port for plaintext channels

Is it worth documenting vnc, when we just stated earlier that protocol
must be spice?

> +# @tls-port:     #optional spice tcp port for tls-secured channels
> +# @cert-subject: #optional server certificate subject
> +#
> +# Since: 0.14.0

So this 'Since:' designation is correct, and we are just _finally_
documenting something that has silently been sitting around in QMP for a
looooong time.

> +##
> +{ 'command': 'client_migrate_info',
> +  'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int',
> +            '*tls-port': 'int', '*cert-subject': 'str' } }

Idea for followups - since 'protocol' must be "spice", should we:
1) make it an enum type rather than open-coded str
2) make it an optional parameter, so that omitting it defaults to spice?

> +
> +##
>  # @MouseInfo:
>  #
>  # Information about a mouse device.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index c267c89..4611b6b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -785,7 +785,7 @@ EQMP
>          .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>          .params     = "protocol hostname port tls-port cert-subject",
>          .help       = "send migration info to spice/vnc client",
> -        .mhandler.cmd_new = client_migrate_info,
> +        .mhandler.cmd_new = qmp_marshal_input_client_migrate_info,

How many entries in qmp-commands.hx are NOT present in qapi? It looks
like this patch gets us one step closer to ditching the need to maintain
this file, which is a good thing.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 05/20] monitor: Use traditional command interface for HMP drive_del

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> All QMP commands use the "new" handler interface (mhandler.cmd_new).
> Most HMP commands still use the traditional interface (mhandler.cmd),
> but a few use the "new" one.  Complicates handle_user_command() for no
> gain, so I'm converting these to the traditional interface.
> 
> For drive_del, that's easy: hmp_drive_del() sheds its unused last
> parameter, and its return value, which the caller ignored anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  blockdev.c                | 9 ++++-----
>  hmp-commands.hx           | 3 +--
>  include/sysemu/blockdev.h | 2 +-
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 06/20] monitor: Use traditional command interface for HMP device_add

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> All QMP commands use the "new" handler interface (mhandler.cmd_new).
> Most HMP commands still use the traditional interface (mhandler.cmd),
> but a few use the "new" one.  Complicates handle_user_command() for no
> gain, so I'm converting these to the traditional interface.
> 
> For device_add, that's easy: just wrap the obvious hmp_device_add()
> around do_device_add().
> 
> monitor_user_noop() is now unused, drop it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hmp-commands.hx | 3 +--
>  hmp.c           | 6 ++++++
>  hmp.h           | 1 +
>  monitor.c       | 2 --
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> All QMP commands use the "new" handler interface (mhandler.cmd_new).
> Most HMP commands still use the traditional interface (mhandler.cmd),
> but a few use the "new" one.  Complicates handle_user_command() for no
> gain, so I'm converting these to the traditional interface.
> 
> pcie_aer_inject_error's implementation is split into the
> hmp_pcie_aer_inject_error() and pcie_aer_inject_error_print().  The
> former is a peculiar crossbreed between HMP and QMP handler.  On
> success, it works like a QMP handler: store QDict through ret_data
> parameter, return 0.  Printing the QDict is left to
> pcie_aer_inject_error_print().  On failure, it works more like an HMP
> handler: print error to monitor, return negative number.
> 
> To convert to the traditional interface, turn
> pcie_aer_inject_error_print() into a command handler wrapping around
> hmp_pcie_aer_inject_error().  By convention, this command handler
> should be called hmp_pcie_aer_inject_error(), so rename the existing
> hmp_pcie_aer_inject_error() to do_pcie_aer_inject_error().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hmp-commands.hx         |  3 +--
>  hw/pci/pci-stub.c       | 14 +-------------
>  hw/pci/pcie_aer.c       | 39 ++++++++++++++++++++++-----------------
>  include/sysemu/sysemu.h |  4 +---
>  4 files changed, 25 insertions(+), 35 deletions(-)
> 

> +
> +    devfn = (int)qdict_get_int(qdict, "devfn");

Code motion, so the problem is pre-existing, but this cast is unneeded.
 Up to you if you want to clean it up now.

> +    monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n",
> +                   qdict_get_str(qdict, "id"),
> +                   qdict_get_str(qdict, "root_bus"),
> +                   (int) qdict_get_int(qdict, "bus"),

However, this one still is necessary, unless you tweak the format string
(if you haven't guessed, I prefer avoiding casts when other mechanisms
work equally well).

But that's minor; whether or not you clean things up,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 08/20] monitor: Drop unused "new" HMP command interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 22 +---------------------
>  1 file changed, 1 insertion(+), 21 deletions(-)
> 

Goodbye!  Nice knowing you!  (For how many incomplete conversions we
have scattered throughout the tree, it's interesting to know that this
one was easier to get rid of by reverting instead of finishing the
conversion)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 09/20] monitor: Propagate errors through qmp_check_client_args()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 412 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 65 ++++++++++++++++++++++++++++++++-------------------------------
>  1 file changed, 33 insertions(+), 32 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 10/20] monitor: Propagate errors through qmp_check_input_obj()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 

> @@ -4948,27 +4948,27 @@ static QDict *qmp_check_input_obj(QObject *input_obj)

>              }
>          } else if (!strcmp(arg_name, "id")) {
>              /* FIXME: check duplicated IDs for async commands */

Is this comment dead, now that you killed async commands?  If so, should
it be nuked earlier in the series?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 11/20] monitor: Wean monitor_protocol_emitter() off mon->error

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Move mon->error handling to its caller handle_qmp_command().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 12/20] monitor: Inline monitor_has_error() into its only caller

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index c732203..71ca03f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -377,11 +377,6 @@ static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,
>      return 0;
>  }
>  
> -static inline int monitor_has_error(const Monitor *mon)
> -{
> -    return mon->error != NULL;

Yay - gets rid of a function returning 0 or 1 instead of a proper bool :)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 13/20] monitor: Limit QError use to command handlers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2622 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> The previous commits narrowed use of QError to handle_qmp_command()
> and its helpers monitor_protocol_emitter(), build_qmp_error_dict().
> Narrow it further to just the command handler call: instead of
> converting Error to QError throughout handle_qmp_command(), convert
> the QError gotten from the command handler to Error, and switch the
> helpers from QError to Error.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 71ca03f..65ef400 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
>      QDECREF(json);
>  }
>  
> -static QDict *build_qmp_error_dict(const QError *err)
> +static QDict *build_qmp_error_dict(Error *err)
>  {
>      QObject *obj;
>  
> -    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
> -                             ErrorClass_lookup[err->err_class],
> -                             qerror_human(err));
> +    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
> +                             ErrorClass_lookup[error_get_class(err)],
> +                             error_get_pretty(err));

You're getting rid of one of three uses of '_jsonf.*%p' in the tree (two
in monitor.c, one in tests/check-qjson.c) [I didn't check for multi-line
instances where the format string was on a different line than the
function call].  Is it worth completely getting rid of the %p formatter,
and simplifying qobject/json-*.c accordingly, in a later patch?

[oh, and I hate that I could not find documentation on the format
strings accepted by qobject_from_jsonf; it took me forever to track down
that %i correlated to bool, when I was doing my series for "qobject" Use
'bool' for qbool]

> @@ -4984,13 +4984,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>      obj = json_parser_parse(tokens, NULL);
>      if (!obj) {
>          // FIXME: should be triggered in json_parser_parse()
> -        qerror_report(QERR_JSON_PARSING);
> +        error_set(&local_err, QERR_JSON_PARSING);
>          goto err_out;

Is this FIXME still valid, or are we reaching the point where it could
be replaced by an assert(obj) in a later patch?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 14/20] monitor: Rename handle_user_command() to handle_hmp_command()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 15/20] monitor: Rename monitor_control_read(), monitor_control_event()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> ... to monitor_qmp_read(), monitor_qmp_event().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

Not a trivial rename based on diffstat, but...

> 
> diff --git a/monitor.c b/monitor.c
> index ecadda7..977c0fd 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -5043,10 +5043,7 @@ err_out:
>      QDECREF(args);
>  }
>  
> -/**
> - * monitor_control_read(): Read and handle QMP input
> - */
> -static void monitor_control_read(void *opaque, const uint8_t *buf, int size)
> +static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)

Drops a comment, but the comment didn't add much.

>  {
>      Monitor *old_mon = cur_mon;
>  
> @@ -5111,10 +5108,7 @@ static QObject *get_qmp_greeting(void)
>      return qobject_from_jsonf("{'QMP':{'version': %p,'capabilities': []}}",ver);
>  }
>  
> -/**
> - * monitor_control_event(): Print QMP gretting
> - */
> -static void monitor_control_event(void *opaque, int event)
> +static void monitor_qmp_event(void *opaque, int event)

Drops a typo in the otherwise useless comment.  (I'm surprised numerous
scans for spelling cleanups haven't yet flagged "gretting" as unusual).

...so I'm okay with it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 16/20] monitor: Unbox Monitor member mc and rename to qmp

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> While there, rename its type as well, from MonitorControl to
> MonitorQMP.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 35 ++++++++++++++++-------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 

> @@ -5255,14 +5255,11 @@ void monitor_init(CharDriverState *chr, int flags)
>          monitor_read_command(mon, 0);
>      }
>  
> -    if (monitor_ctrl_mode(mon)) {
> -        mon->mc = g_malloc0(sizeof(MonitorControl));
> -        /* Control mode requires special handlers */
> +    if (flags & MONITOR_USE_CONTROL) {

You also inlined this use of monitor_ctrl_mode; worth mentioning in the
commit message?  Or should it have been swapped to monitor_is_qmp() in
patch 19?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 17/20] monitor: Drop do_qmp_capabilities()'s superfluous QMP check

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Superfluous since commit 30f5041 removed it from HMP.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 18/20] monitor: Turn int command_mode into bool in_command_mode

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 

Might be worth mentioning in the commit message that...

>  
> -static inline int qmp_cmd_mode(const Monitor *mon)
> -{
> -    return mon->qmp.command_mode;
> -}

...qmp_cmd_mode() is now inlined into its callers.

Nice, fixes up a mis-use of 'int' that I complained about when writing
commit 2d5a8346

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 19/20] monitor: Rename monitor_ctrl_mode() to monitor_is_qmp()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> ... and change return type to bool.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 

See also my review on 16 about a usage in monitor_init().

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 20/20] monitor: Change return type of monitor_cur_is_qmp() to bool

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 683 bytes --]

On 05/22/2015 05:36 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/monitor/monitor.h | 2 +-
>  monitor.c                 | 6 ++++--
>  stubs/mon-is-qmp.c        | 2 +-
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 

> +++ b/stubs/mon-is-qmp.c
> @@ -1,7 +1,7 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>  
> -int monitor_cur_is_qmp(void)
> +bool monitor_cur_is_qmp(void)
>  {
>      return 0;

s/0/false/

with that change,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 01/20] monitor: Drop broken, unused asynchronous command interface

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> The asynchronous monitor command interface goes back to commit 940cc30
>> (Jan 2010).  Added a third case to command execution.  The hope back
>> then according to the commit message was that all commands get
>> converted to the asynchronous interface, killing off the other two
>> cases.  Didn't happen.
>> 
>> The initial asynchronous commands balloon and info balloon were
>> converted back to synchronous long ago (commit 96637bc and d72f32),
>> with commit messages calling the asynchronous interface "not fully
>> working" and "deprecated".  The only other user went away in commit
>> 3b5704b.
>
> The history is helpful; thanks.
>
>> 
>> New code generally uses synchronous commands and asynchronous events.
>> 
>> What exactly is still "not fully working" with asynchronous commands?
>> Well, here's a bug that defeats actual asynchronous use pretty
>> reliably: the reply's ID is wrong (and has always been wrong) unless
>> you use the command synchronously!  To reproduce, we need an
>> asynchronous command, so we have to go back before the commit 3b5704b.
>> Run QEMU with spice:
>> 
>>     $ qemu-system-x86_64 -nodefaults -S -spice
>> port=5900,disable-ticketing -qmp stdio
>>     {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major":
>> 2}, "package": ""}, "capabilities": []}}
>> 
>> Connect a spice client in another terminal:
>> 
>>     $ remote-viewer spice://localhost:5900
>> 
>> Set up a migration destination dummy in a third terminal:
>> 
>>     $ socat TCP-LISTEN:12345 STDIO
>> 
>> Now paste the following into the QMP monitor:
>> 
>>     { "execute": "qmp_capabilities" }
>
> If you stick and "id":"i0" here...
>
>>     { "execute": "client_migrate_info", "id": "i1", "arguments": {
>> "protocol": "spice", "hostname": "localhost", "port": 12345 } }
>>     { "execute": "query-kvm", "id": "i2" }
>> 
>> Produces two replies immediately, one to qmp_capabilities, and one to
>> query-kvm:
>> 
>>     {"return": {}}
>
> ...it should show up here, for even less ambiguity about the problems
> with the async response.

Can do.

>>     {"return": {"enabled": false, "present": true}, "id": "i2"}
>> 
>> Both are correct.  Two lines of debug output from libspice-server not
>> shown.
>> 
>> Now EOF socat's standard input to make it close the connection.  This
>> makes the asynchronous client_migrate_info complete.  It replies:
>> 
>>     {"return": {}}
>> 
>> Bug: "id": "i1" is missing.  Two lines of debug output from
>> libspice-server not shown.  Cherry on top: storage for the missing ID
>> is leaked.
>> 
>> Get rid of this stuff before somebody hurts himself with it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/monitor/monitor.h |  5 ----
>>  monitor.c                 | 68 ++---------------------------------------------
>>  2 files changed, 2 insertions(+), 71 deletions(-)
>
> Certainly takes an axe to it!
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 02/20] monitor: Clean up after previous commit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> Commit message is sparse; I would have mentioned [1] and [2].
>
>>  monitor.c | 40 +++++++++++-----------------------------
>>  1 file changed, 11 insertions(+), 29 deletions(-)
>> 
>>  
>> -static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
>> -{
>> -    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);
>> -    }
>> -}
>
> [2] - handler_audit() goes away now that it has no caller
>
>> -
>>  static void handle_user_command(Monitor *mon, const char *cmdline)
>>  {
>>      QDict *qdict;
>> @@ -5015,28 +5003,17 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
>>      return input_dict;
>>  }
>>  
>> -static void qmp_call_cmd(Monitor *mon, const mon_cmd_t *cmd,
>> -                         const QDict *params)
>> -{
>> -    int ret;
>> -    QObject *data = NULL;
>> -
>> -    ret = cmd->mhandler.cmd_new(mon, params, &data);
>> -    handler_audit(mon, cmd, ret);
>> -    monitor_protocol_emitter(mon, data);
>> -    qobject_decref(data);
>> -}
>
> [1] - qmp_call_cmd() is inlined and simplified...
>
>> -
>>  static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>  {
>>      int err;
>> -    QObject *obj;
>> +    QObject *obj, *data;
>>      QDict *input, *args;
>>      const mon_cmd_t *cmd;
>>      const char *cmd_name;
>>      Monitor *mon = cur_mon;
>>  
>>      args = input = NULL;
>> +    data = NULL;
>>  
>>      obj = json_parser_parse(tokens, NULL);
>>      if (!obj) {
>> @@ -5079,12 +5056,17 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>          goto err_out;
>>      }
>>  
>> -    qmp_call_cmd(mon, cmd, args);
>> -    goto out;
>> +    if (cmd->mhandler.cmd_new(mon, args, &data)) {
>> +        /* Command failed... */
>> +        if (!monitor_has_error(mon)) {
>> +            /* ... without setting an error, so make one up */
>> +            qerror_report(QERR_UNDEFINED_ERROR);
>> +        }
>> +    }
>
> ...into its lone caller.

Can do.

>>  
>>  err_out:
>> -    monitor_protocol_emitter(mon, NULL);
>> -out:
>> +    monitor_protocol_emitter(mon, data);
>> +    qobject_decref(data);
>>      QDECREF(input);
>>      QDECREF(args);
>>  }
>> 
>
> But the code cleanup itself looks sane, so with the improved commit message:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hmp-commands.hx  |  3 +--
>>  hmp.c            | 17 +++++++++++++++++
>>  hmp.h            |  1 +
>>  monitor.c        | 42 ++++++++++++++++++------------------------
>>  qapi-schema.json | 20 ++++++++++++++++++++
>>  qmp-commands.hx  |  2 +-
>>  6 files changed, 58 insertions(+), 27 deletions(-)
>> 
>
>> +++ b/qapi-schema.json
>> @@ -638,6 +638,26 @@
>>    'returns': 'MigrationParameters' }
>>  
>>  ##
>> +# @client_migrate_info
>
> Should we name this new command 'client-migrate-info'?
>
> /me goes and checks...
>
> Oh, wow, the QMP command IS advertised by 'query-commands' in spite of
> not having this wrapper, even in qemu 2.3.

Yup.  query-commands uses qmp_cmds[], not QAPI.

>> +#
>> +# Set the spice/vnc connection info for the migration target.  The
>> +# spice/vnc server will ask the spice/vnc client to automatically
>> +# reconnect using the new parameters (if specified) once the vm
>> +# migration finished successfully.  Not yet implemented for VNC.
>> +#
>> +# @protocol:     must be "spice"
>> +# @hostname:     migration target hostname
>> +# @port:         #optional spice/vnc tcp port for plaintext channels
>
> Is it worth documenting vnc, when we just stated earlier that protocol
> must be spice?

I think this is a question for Gerd (cc'ed).

>> +# @tls-port:     #optional spice tcp port for tls-secured channels
>> +# @cert-subject: #optional server certificate subject
>> +#
>> +# Since: 0.14.0
>
> So this 'Since:' designation is correct, and we are just _finally_
> documenting something that has silently been sitting around in QMP for a
> looooong time.

It's always been documented in qmp-commands.hx.

qmp-commands.hx predates QAPI.  We still use it to generate
qmp-commands.txt and qmp-commands-old.h.  The latter provides the
initializer for qmp_cmds[], thus determines the reply to query-commands.

qmp-commands.hx is complete.  The QAPI schema contains only qapified
commands, and therefore cannot serve as single authoritative source.

An awful lot of information is duplicated between qmp-commands.hx and
the QAPI schema.  To clean up this mess, we need to finish qapifying
commands, and then fold qmp-commands.hx's functionality into the schema.

>> +##
>> +{ 'command': 'client_migrate_info',
>> +  'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int',
>> +            '*tls-port': 'int', '*cert-subject': 'str' } }
>
> Idea for followups - since 'protocol' must be "spice", should we:
> 1) make it an enum type rather than open-coded str

Useful cleanup.

> 2) make it an optional parameter, so that omitting it defaults to spice?

Not sure.  Gerd?

>> +
>> +##
>>  # @MouseInfo:
>>  #
>>  # Information about a mouse device.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index c267c89..4611b6b 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -785,7 +785,7 @@ EQMP
>>          .args_type  = "protocol:s,hostname:s,port:i?,tls-port:i?,cert-subject:s?",
>>          .params     = "protocol hostname port tls-port cert-subject",
>>          .help       = "send migration info to spice/vnc client",
>> -        .mhandler.cmd_new = client_migrate_info,
>> +        .mhandler.cmd_new = qmp_marshal_input_client_migrate_info,
>
> How many entries in qmp-commands.hx are NOT present in qapi?

Not many, but I don't remember the exact list offhand.  The most
difficult one is probably device_add.  I got patches getting that one a
bit closer, but they depend by a straightforward PPC patch I've been
trying to get in since February.

>                                                              It looks
> like this patch gets us one step closer to ditching the need to maintain
> this file, which is a good thing.

Yes.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> All QMP commands use the "new" handler interface (mhandler.cmd_new).
>> Most HMP commands still use the traditional interface (mhandler.cmd),
>> but a few use the "new" one.  Complicates handle_user_command() for no
>> gain, so I'm converting these to the traditional interface.
>> 
>> pcie_aer_inject_error's implementation is split into the
>> hmp_pcie_aer_inject_error() and pcie_aer_inject_error_print().  The
>> former is a peculiar crossbreed between HMP and QMP handler.  On
>> success, it works like a QMP handler: store QDict through ret_data
>> parameter, return 0.  Printing the QDict is left to
>> pcie_aer_inject_error_print().  On failure, it works more like an HMP
>> handler: print error to monitor, return negative number.
>> 
>> To convert to the traditional interface, turn
>> pcie_aer_inject_error_print() into a command handler wrapping around
>> hmp_pcie_aer_inject_error().  By convention, this command handler
>> should be called hmp_pcie_aer_inject_error(), so rename the existing
>> hmp_pcie_aer_inject_error() to do_pcie_aer_inject_error().
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hmp-commands.hx         |  3 +--
>>  hw/pci/pci-stub.c       | 14 +-------------
>>  hw/pci/pcie_aer.c       | 39 ++++++++++++++++++++++-----------------
>>  include/sysemu/sysemu.h |  4 +---
>>  4 files changed, 25 insertions(+), 35 deletions(-)
>> 
>
>> +
>> +    devfn = (int)qdict_get_int(qdict, "devfn");
>
> Code motion, so the problem is pre-existing, but this cast is unneeded.
>  Up to you if you want to clean it up now.

While I occasionally do fold cleanups into mechanical changes, e.g in
PATCH 15, I'm reluctant to fold them into code motion.

>> +    monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n",
>> +                   qdict_get_str(qdict, "id"),
>> +                   qdict_get_str(qdict, "root_bus"),
>> +                   (int) qdict_get_int(qdict, "bus"),
>
> However, this one still is necessary, unless you tweak the format string
> (if you haven't guessed, I prefer avoiding casts when other mechanisms
> work equally well).

I share your preference, but I want to keep the changes in PCI code to a
trivial minimum, to keep this series firmly in the monitor subsystem.

> But that's minor; whether or not you clean things up,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 10/20] monitor: Propagate errors through qmp_check_input_obj()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>> 
>
>> @@ -4948,27 +4948,27 @@ static QDict *qmp_check_input_obj(QObject *input_obj)
>
>>              }
>>          } else if (!strcmp(arg_name, "id")) {
>>              /* FIXME: check duplicated IDs for async commands */
>
> Is this comment dead, now that you killed async commands?  If so, should
> it be nuked earlier in the series?

I figure you're right.  I'll kill the comment together with asynchronous
commands.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 13/20] monitor: Limit QError use to command handlers

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> The previous commits narrowed use of QError to handle_qmp_command()
>> and its helpers monitor_protocol_emitter(), build_qmp_error_dict().
>> Narrow it further to just the command handler call: instead of
>> converting Error to QError throughout handle_qmp_command(), convert
>> the QError gotten from the command handler to Error, and switch the
>> helpers from QError to Error.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 71ca03f..65ef400 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -391,19 +391,19 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
>>      QDECREF(json);
>>  }
>>  
>> -static QDict *build_qmp_error_dict(const QError *err)
>> +static QDict *build_qmp_error_dict(Error *err)
>>  {
>>      QObject *obj;
>>  
>> -    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %p } }",
>> -                             ErrorClass_lookup[err->err_class],
>> -                             qerror_human(err));
>> +    obj = qobject_from_jsonf("{ 'error': { 'class': %s, 'desc': %s } }",
>> +                             ErrorClass_lookup[error_get_class(err)],
>> +                             error_get_pretty(err));
>
> You're getting rid of one of three uses of '_jsonf.*%p' in the tree (two
> in monitor.c, one in tests/check-qjson.c) [I didn't check for multi-line
> instances where the format string was on a different line than the
> function call].  Is it worth completely getting rid of the %p formatter,
> and simplifying qobject/json-*.c accordingly, in a later patch?

No idea :)

Here, %p removal simply falls out of the QError -> Error conversion.  I
don't know how it's used elsewhere, and whether the uses are worth the
code backing it.

> [oh, and I hate that I could not find documentation on the format
> strings accepted by qobject_from_jsonf; it took me forever to track down
> that %i correlated to bool, when I was doing my series for "qobject" Use
> 'bool' for qbool]

Valid complaint.  Pretty much everything from that particular hacking
bout is underdocumented, but the undocumented conversion specifiers are
particularly egregious.

>> @@ -4984,13 +4984,12 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens)
>>      obj = json_parser_parse(tokens, NULL);
>>      if (!obj) {
>>          // FIXME: should be triggered in json_parser_parse()
>> -        qerror_report(QERR_JSON_PARSING);
>> +        error_set(&local_err, QERR_JSON_PARSING);
>>          goto err_out;
>
> Is this FIXME still valid, or are we reaching the point where it could
> be replaced by an assert(obj) in a later patch?

Yes, it's still valid.  Watch this:

    {"QMP": {"version": {"qemu": {"micro": 94, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
    { "execute": "\xxxxxxxxx" }
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    { "execute": "qmp_capabilities" }
    {"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Expected 'object' in QMP input"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}
    {"error": {"class": "GenericError", "desc": "Invalid JSON syntax"}}

This does enter json-parser.c's

                        parse_error(ctxt, token,
                                    "invalid hex escape sequence in string");

and parse_error() duly creates an Error object with error_setg(), but
the object then gets lost somewhere on the way.

Also visible: multiple error replies, which is wrong, and an apparent
inability to recover from syntax error.

Not just the documentation sucks.  But then half-assed documentation has
always been a pretty reliable indicator of half-assed code.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 16/20] monitor: Unbox Monitor member mc and rename to qmp

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> While there, rename its type as well, from MonitorControl to
>> MonitorQMP.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 35 ++++++++++++++++-------------------
>>  1 file changed, 16 insertions(+), 19 deletions(-)
>> 
>
>> @@ -5255,14 +5255,11 @@ void monitor_init(CharDriverState *chr, int flags)
>>          monitor_read_command(mon, 0);
>>      }
>>  
>> -    if (monitor_ctrl_mode(mon)) {
>> -        mon->mc = g_malloc0(sizeof(MonitorControl));
>> -        /* Control mode requires special handlers */
>> +    if (flags & MONITOR_USE_CONTROL) {
>
> You also inlined this use of monitor_ctrl_mode; worth mentioning in the
> commit message?  Or should it have been swapped to monitor_is_qmp() in
> patch 19?

I'll do either one or the other.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 18/20] monitor: Turn int command_mode into bool in_command_mode

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>> 
>
> Might be worth mentioning in the commit message that...
>
>>  
>> -static inline int qmp_cmd_mode(const Monitor *mon)
>> -{
>> -    return mon->qmp.command_mode;
>> -}
>
> ...qmp_cmd_mode() is now inlined into its callers.

Can do.

> Nice, fixes up a mis-use of 'int' that I complained about when writing
> commit 2d5a8346
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 20/20] monitor: Change return type of monitor_cur_is_qmp() to bool

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/monitor/monitor.h | 2 +-
>>  monitor.c                 | 6 ++++--
>>  stubs/mon-is-qmp.c        | 2 +-
>>  3 files changed, 6 insertions(+), 4 deletions(-)
>> 
>
>> +++ b/stubs/mon-is-qmp.c
>> @@ -1,7 +1,7 @@
>>  #include "qemu-common.h"
>>  #include "monitor/monitor.h"
>>  
>> -int monitor_cur_is_qmp(void)
>> +bool monitor_cur_is_qmp(void)
>>  {
>>      return 0;
>
> s/0/false/

Yes.

> with that change,
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Gerd Hoffmann]

  Hi,

> >> +#
> >> +# Set the spice/vnc connection info for the migration target.  The
> >> +# spice/vnc server will ask the spice/vnc client to automatically
> >> +# reconnect using the new parameters (if specified) once the vm
> >> +# migration finished successfully.  Not yet implemented for VNC.
> >> +#
> >> +# @protocol:     must be "spice"
> >> +# @hostname:     migration target hostname
> >> +# @port:         #optional spice/vnc tcp port for plaintext channels
> >
> > Is it worth documenting vnc, when we just stated earlier that protocol
> > must be spice?
> 
> I think this is a question for Gerd (cc'ed).

IIRC Daniel (added to Cc:) had plans to create a vnc extension for that.

Which was the reason to explicitly add the protocol here, so we can use
the same command for both spice and vnc some day.

> >> +##
> >> +{ 'command': 'client_migrate_info',
> >> +  'data': { 'protocol': 'str', 'hostname': 'str', '*port': 'int',
> >> +            '*tls-port': 'int', '*cert-subject': 'str' } }
> >
> > Idea for followups - since 'protocol' must be "spice", should we:
> > 1) make it an enum type rather than open-coded str
> 
> Useful cleanup.

Agree.

> > 2) make it an optional parameter, so that omitting it defaults to spice?
> 
> Not sure.  Gerd?

Not sure this buys us much.  I guess libvirt will continue to pass
'spice' because everything else doesn't make much sense.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Daniel P. Berrange]

On Tue, May 26, 2015 at 02:51:45PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > >> +#
> > >> +# Set the spice/vnc connection info for the migration target.  The
> > >> +# spice/vnc server will ask the spice/vnc client to automatically
> > >> +# reconnect using the new parameters (if specified) once the vm
> > >> +# migration finished successfully.  Not yet implemented for VNC.
> > >> +#
> > >> +# @protocol:     must be "spice"
> > >> +# @hostname:     migration target hostname
> > >> +# @port:         #optional spice/vnc tcp port for plaintext channels
> > >
> > > Is it worth documenting vnc, when we just stated earlier that protocol
> > > must be spice?
> > 
> > I think this is a question for Gerd (cc'ed).
> 
> IIRC Daniel (added to Cc:) had plans to create a vnc extension for that.
> 
> Which was the reason to explicitly add the protocol here, so we can use
> the same command for both spice and vnc some day.

Yeah, it would be nice to do a VNC extension, though realistically I'm
not going to have time for that anytime in the forseeable future.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 07/20] monitor: Use trad. command interface for HMP pcie_aer_inject_error

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On 05/26/2015 03:25 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 05/22/2015 05:36 AM, Markus Armbruster wrote:
>>> All QMP commands use the "new" handler interface (mhandler.cmd_new).
>>> Most HMP commands still use the traditional interface (mhandler.cmd),
>>> but a few use the "new" one.  Complicates handle_user_command() for no
>>> gain, so I'm converting these to the traditional interface.
>>>

>>
>>> +
>>> +    devfn = (int)qdict_get_int(qdict, "devfn");
>>
>> Code motion, so the problem is pre-existing, but this cast is unneeded.
>>  Up to you if you want to clean it up now.
> 
> While I occasionally do fold cleanups into mechanical changes, e.g in
> PATCH 15, I'm reluctant to fold them into code motion.

I totally agree - mixing cleanups into code motion makes the motion
harder to review.  If it gets cleaned up, it should be as a separate
patch, at which point it starts to be out of scope of the goal of this
series, and better as a separate patch to qemu-trivial or to the PCI tree.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, May 26, 2015 at 02:51:45PM +0200, Gerd Hoffmann wrote:
>>   Hi,
>> 
>> > >> +#
>> > >> +# Set the spice/vnc connection info for the migration target.  The
>> > >> +# spice/vnc server will ask the spice/vnc client to automatically
>> > >> +# reconnect using the new parameters (if specified) once the vm
>> > >> +# migration finished successfully.  Not yet implemented for VNC.
>> > >> +#
>> > >> +# @protocol:     must be "spice"
>> > >> +# @hostname:     migration target hostname
>> > >> +# @port:         #optional spice/vnc tcp port for plaintext channels
>> > >
>> > > Is it worth documenting vnc, when we just stated earlier that protocol
>> > > must be spice?
>> > 
>> > I think this is a question for Gerd (cc'ed).
>> 
>> IIRC Daniel (added to Cc:) had plans to create a vnc extension for that.
>> 
>> Which was the reason to explicitly add the protocol here, so we can use
>> the same command for both spice and vnc some day.
>
> Yeah, it would be nice to do a VNC extension, though realistically I'm
> not going to have time for that anytime in the forseeable future.

Should we continue to document the command is about "spice/vnc
connection info", or should we rephrase?  Something like:

client_migrate_info
-------------------

Set remote display connection information for migration.  This makes the
server ask the client to automatically reconnect using the new
parameters once migration finished successfully.  Only implemented for
SPICE.

Arguments:

- "protocol":     must be "spice" (json-string)
- "hostname":     migration target hostname (json-string)
- "port":         spice/vnc tcp port for plaintext channels (json-int, optional)
- "tls-port":     spice tcp port for tls-secured channels (json-int, optional)
- "cert-subject": server certificate subject (json-string, optional)

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Gerd Hoffmann]

  Hi,

> Should we continue to document the command is about "spice/vnc
> connection info", or should we rephrase?  Something like:
> 
> client_migrate_info
> -------------------
> 
> Set remote display connection information for migration.  This makes the
> server ask the client to automatically reconnect using the new
> parameters once migration finished successfully.  Only implemented for
> SPICE.
> 
> Arguments:
> 
> - "protocol":     must be "spice" (json-string)
> - "hostname":     migration target hostname (json-string)
> - "port":         spice/vnc tcp port for plaintext channels (json-int, optional)
> - "tls-port":     spice tcp port for tls-secured channels (json-int, optional)
> - "cert-subject": server certificate subject (json-string, optional)

Looks good to me.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 04/20] monitor: Convert client_migrate_info to QAPI

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> Should we continue to document the command is about "spice/vnc
>> connection info", or should we rephrase?  Something like:
>> 
>> client_migrate_info
>> -------------------
>> 
>> Set remote display connection information for migration.  This makes the
>> server ask the client to automatically reconnect using the new
>> parameters once migration finished successfully.  Only implemented for
>> SPICE.
>> 
>> Arguments:
>> 
>> - "protocol":     must be "spice" (json-string)
>> - "hostname":     migration target hostname (json-string)
>> - "port": spice/vnc tcp port for plaintext channels (json-int,
>> optional)
>> - "tls-port":     spice tcp port for tls-secured channels (json-int, optional)
>> - "cert-subject": server certificate subject (json-string, optional)
>
> Looks good to me.

Okay, I'll squash something like that into PATCH 03, and cc: you for
review.