[Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Luiz Capitulino]

 Hi there,

 When I started converting handlers to the QObject style, I thought that
returning an error code wouldn't be needed. That is, we have an error object
already, so if the handler returns the error object it has failed, otherwise
it has succeeded.

 This was also very convenient, because handlers have never returned an error
code, and thus it would be easier to add a call to qemu_error_new() in the
right place instead of returning error codes.

 Turns out we need both. Actually, I should not have abused the error object
this way because (as Markus says) this is too fragile and we can end up
reporting bogus errors to clients (among other problems).

 The good news is that it's easy to fix.

 All we have to do is to change cmd_new() (the handler callback) to return an
error code and convert existing QObject handlers to it. This series does that
and most of the patches are really straightforward conversions.

 Additionally, Markus has designed an excellent debug mechanism for QMP, which
is implemented by the last patches in this series, and will allow us to catch
important QObject conversion and error handling bugs in handlers.

 Thanks.

[Qemu-devel] [PATCH 01/21] Monitor: Introduce cmd_new_ret()

[Luiz Capitulino]

In order to implement the new error handling and debugging
mechanism for command handlers, we need to change the cmd_new()
callback to return a value.

This commit introduces cmd_new_ret(), which returns a value and
will be used only temporarily to handle the transition from
cmd_new().

That is, as soon as all command handlers are ported to cmd_new_ret(),
it will be renamed back to cmd_new() and the new error handling
and debugging mechanism will be added on top of it.

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

diff --git a/monitor.c b/monitor.c
index ae125b8..63c62fb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -98,6 +98,7 @@ typedef struct mon_cmd_t {
     const char *params;
     const char *help;
     void (*user_print)(Monitor *mon, const QObject *data);
+    int (*cmd_new_ret)(Monitor *mon, const QDict *params, QObject **ret_data);
     union {
         void (*info)(Monitor *mon);
         void (*info_new)(Monitor *mon, QObject **ret_data);
@@ -3801,7 +3802,11 @@ static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
 {
     QObject *data = NULL;
 
-    cmd->mhandler.cmd_new(mon, params, &data);
+    if (cmd->cmd_new_ret) {
+        cmd->cmd_new_ret(mon, params, &data);
+    } else {
+        cmd->mhandler.cmd_new(mon, params, &data);
+    }
 
     if (is_async_return(data)) {
         /*
-- 
1.6.6

Re: [Qemu-devel] [PATCH 01/21] Monitor: Introduce cmd_new_ret()

[Anthony Liguori]

On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
> In order to implement the new error handling and debugging
> mechanism for command handlers, we need to change the cmd_new()
> callback to return a value.
>
> This commit introduces cmd_new_ret(), which returns a value and
> will be used only temporarily to handle the transition from
> cmd_new().
>
> That is, as soon as all command handlers are ported to cmd_new_ret(),
> it will be renamed back to cmd_new() and the new error handling
> and debugging mechanism will be added on top of it.
>
> Signed-off-by: Luiz Capitulino<lcapitulino@redhat.com>
>    

Applied all.  Thanks!

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 02/21] Monitor: Convert simple handlers to cmd_new_ret()

[Luiz Capitulino]

The following handlers always succeed and hence can be converted
to cmd_new_ret() in the same commit.

- do_stop()
- do_quit()
- do_system_reset()
- do_system_powerdown()
- do_migrate_cancel()
- do_qmp_capabilities()
- do_migrate_set_speed()
- do_migrate_set_downtime()

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 migration.c     |   14 ++++++++++----
 migration.h     |    8 ++++----
 monitor.c       |   22 ++++++++++++++--------
 qemu-monitor.hx |   16 ++++++++--------
 4 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/migration.c b/migration.c
index 2320c5f..557bec4 100644
--- a/migration.c
+++ b/migration.c
@@ -98,15 +98,17 @@ void do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     }
 }
 
-void do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
+int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = current_migration;
 
     if (s)
         s->cancel(s);
+
+    return 0;
 }
 
-void do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
+int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     double d;
     FdMigrationState *s;
@@ -119,6 +121,8 @@ void do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (s && s->file) {
         qemu_file_set_rate_limit(s->file, max_throttle);
     }
+
+    return 0;
 }
 
 /* amount of nanoseconds we are willing to wait for migration to be down.
@@ -132,14 +136,16 @@ uint64_t migrate_max_downtime(void)
     return max_downtime;
 }
 
-void do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
-                             QObject **ret_data)
+int do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
+                            QObject **ret_data)
 {
     double d;
 
     d = qdict_get_double(qdict, "value") * 1e9;
     d = MAX(0, MIN(UINT64_MAX, d));
     max_downtime = (uint64_t)d;
+
+    return 0;
 }
 
 static void migrate_print_status(Monitor *mon, const char *name,
diff --git a/migration.h b/migration.h
index 65572c1..9345d97 100644
--- a/migration.h
+++ b/migration.h
@@ -54,14 +54,14 @@ void qemu_start_incoming_migration(const char *uri);
 
 void do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
-void do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
-void do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_migrate_set_speed(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 uint64_t migrate_max_downtime(void);
 
-void do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
-                             QObject **ret_data);
+int do_migrate_set_downtime(Monitor *mon, const QDict *qdict,
+                            QObject **ret_data);
 
 void do_info_migrate_print(Monitor *mon, const QObject *data);
 
diff --git a/monitor.c b/monitor.c
index 63c62fb..c1e0af8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -417,13 +417,15 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
     QDECREF(qmp);
 }
 
-static void do_qmp_capabilities(Monitor *mon, const QDict *params,
-                                QObject **ret_data)
+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->mc->command_mode = 1;
     }
+
+    return 0;
 }
 
 static int compare_cmd(const char *name, const char *list)
@@ -962,9 +964,10 @@ static void do_info_cpu_stats(Monitor *mon)
 /**
  * do_quit(): Quit QEMU execution
  */
-static void do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_quit(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     exit(0);
+    return 0;
 }
 
 static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
@@ -1129,9 +1132,10 @@ static void do_singlestep(Monitor *mon, const QDict *qdict)
 /**
  * do_stop(): Stop VM execution
  */
-static void do_stop(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_stop(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     vm_stop(EXCP_INTERRUPT);
+    return 0;
 }
 
 static void encrypted_bdrv_it(void *opaque, BlockDriverState *bs);
@@ -1829,19 +1833,21 @@ static void do_boot_set(Monitor *mon, const QDict *qdict)
 /**
  * do_system_reset(): Issue a machine reset
  */
-static void do_system_reset(Monitor *mon, const QDict *qdict,
-                            QObject **ret_data)
+static int do_system_reset(Monitor *mon, const QDict *qdict,
+                           QObject **ret_data)
 {
     qemu_system_reset_request();
+    return 0;
 }
 
 /**
  * do_system_powerdown(): Issue a machine powerdown
  */
-static void do_system_powerdown(Monitor *mon, const QDict *qdict,
-                                QObject **ret_data)
+static int do_system_powerdown(Monitor *mon, const QDict *qdict,
+                               QObject **ret_data)
 {
     qemu_system_powerdown_request();
+    return 0;
 }
 
 #if defined(TARGET_I386)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 7f9d261..8e51df0 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -123,7 +123,7 @@ ETEXI
         .params     = "",
         .help       = "quit the emulator",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_quit,
+        .cmd_new_ret = do_quit,
     },
 
 STEXI
@@ -303,7 +303,7 @@ ETEXI
         .params     = "",
         .help       = "stop emulation",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_stop,
+        .cmd_new_ret = do_stop,
     },
 
 STEXI
@@ -494,7 +494,7 @@ ETEXI
         .params     = "",
         .help       = "reset the system",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_system_reset,
+        .cmd_new_ret = do_system_reset,
     },
 
 STEXI
@@ -510,7 +510,7 @@ ETEXI
         .params     = "",
         .help       = "send system power down event",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_system_powerdown,
+        .cmd_new_ret = do_system_powerdown,
     },
 
 STEXI
@@ -791,7 +791,7 @@ ETEXI
         .params     = "",
         .help       = "cancel the current VM migration",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_migrate_cancel,
+        .cmd_new_ret = do_migrate_cancel,
     },
 
 STEXI
@@ -806,7 +806,7 @@ ETEXI
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_migrate_set_speed,
+        .cmd_new_ret = do_migrate_set_speed,
     },
 
 STEXI
@@ -821,7 +821,7 @@ ETEXI
         .params     = "value",
         .help       = "set maximum tolerated downtime (in seconds) for migrations",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_migrate_set_downtime,
+        .cmd_new_ret = do_migrate_set_downtime,
     },
 
 STEXI
@@ -1132,7 +1132,7 @@ ETEXI
         .params     = "",
         .help       = "enable QMP capabilities",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_qmp_capabilities,
+        .cmd_new_ret = do_qmp_capabilities,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 03/21] Monitor: Convert do_cont() to cmd_new_ret()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    8 ++++++--
 qemu-monitor.hx |    2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index c1e0af8..cede368 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1148,14 +1148,18 @@ struct bdrv_iterate_context {
 /**
  * do_cont(): Resume emulation.
  */
-static void do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_cont(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     struct bdrv_iterate_context context = { mon, 0 };
 
     bdrv_iterate(encrypted_bdrv_it, &context);
     /* only resume the vm if all keys are set and valid */
-    if (!context.err)
+    if (!context.err) {
         vm_start();
+        return 0;
+    } else {
+        return -1;
+    }
 }
 
 static void bdrv_key_cb(void *opaque, int err)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 8e51df0..0eab6db 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -318,7 +318,7 @@ ETEXI
         .params     = "",
         .help       = "resume emulation",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_cont,
+        .cmd_new_ret = do_cont,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 04/21] Monitor: Convert do_eject() to cmd_new_ret()

[Luiz Capitulino]

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

diff --git a/monitor.c b/monitor.c
index cede368..e960c38 100644
--- a/monitor.c
+++ b/monitor.c
@@ -989,7 +989,7 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
     return 0;
 }
 
-static void do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     BlockDriverState *bs;
     int force = qdict_get_int(qdict, "force");
@@ -998,9 +998,9 @@ static void do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
     bs = bdrv_find(filename);
     if (!bs) {
         qemu_error_new(QERR_DEVICE_NOT_FOUND, filename);
-        return;
+        return -1;
     }
-    eject_device(mon, bs, force);
+    return eject_device(mon, bs, force);
 }
 
 static void do_block_set_passwd(Monitor *mon, const QDict *qdict,
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 0eab6db..c1bd773 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -138,7 +138,7 @@ ETEXI
         .params     = "[-f] device",
         .help       = "eject a removable medium (use -f to force it)",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_eject,
+        .cmd_new_ret = do_eject,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 05/21] Monitor: Convert do_cpu_set() to cmd_new_ret()

[Luiz Capitulino]

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

diff --git a/monitor.c b/monitor.c
index e960c38..598dbfe 100644
--- a/monitor.c
+++ b/monitor.c
@@ -921,11 +921,14 @@ static void do_info_cpus(Monitor *mon, QObject **ret_data)
     *ret_data = QOBJECT(cpu_list);
 }
 
-static void do_cpu_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_cpu_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     int index = qdict_get_int(qdict, "index");
-    if (mon_set_cpu(index) < 0)
+    if (mon_set_cpu(index) < 0) {
         qemu_error_new(QERR_INVALID_PARAMETER, "index");
+        return -1;
+    }
+    return 0;
 }
 
 static void do_info_jit(Monitor *mon)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index c1bd773..835fd05 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -604,7 +604,7 @@ ETEXI
         .params     = "index",
         .help       = "set the default CPU",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_cpu_set,
+        .cmd_new_ret = do_cpu_set,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 06/21] Monitor: Convert do_block_set_passwd() to cmd_new_ret()

[Luiz Capitulino]

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

diff --git a/monitor.c b/monitor.c
index 598dbfe..0ae408a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1006,7 +1006,7 @@ static int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return eject_device(mon, bs, force);
 }
 
-static void do_block_set_passwd(Monitor *mon, const QDict *qdict,
+static int do_block_set_passwd(Monitor *mon, const QDict *qdict,
                                 QObject **ret_data)
 {
     BlockDriverState *bs;
@@ -1014,12 +1014,15 @@ static void do_block_set_passwd(Monitor *mon, const QDict *qdict,
     bs = bdrv_find(qdict_get_str(qdict, "device"));
     if (!bs) {
         qemu_error_new(QERR_DEVICE_NOT_FOUND, qdict_get_str(qdict, "device"));
-        return;
+        return -1;
     }
 
     if (bdrv_set_key(bs, qdict_get_str(qdict, "password")) < 0) {
         qemu_error_new(QERR_INVALID_PASSWORD);
+        return -1;
     }
+
+    return 0;
 }
 
 static void do_change_block(Monitor *mon, const char *device,
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 835fd05..d2be5c4 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1117,7 +1117,7 @@ ETEXI
         .params     = "block_passwd device password",
         .help       = "set the password of encrypted block devices",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_block_set_passwd,
+        .cmd_new_ret = do_block_set_passwd,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 07/21] Monitor: Convert do_getfd() to cmd_new_ret()

[Luiz Capitulino]

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

diff --git a/monitor.c b/monitor.c
index 0ae408a..9466c63 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2403,7 +2403,7 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
 }
 #endif
 
-static void do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *fdname = qdict_get_str(qdict, "fdname");
     mon_fd_t *monfd;
@@ -2412,12 +2412,12 @@ static void do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
     fd = qemu_chr_get_msgfd(mon->chr);
     if (fd == -1) {
         qemu_error_new(QERR_FD_NOT_SUPPLIED);
-        return;
+        return -1;
     }
 
     if (qemu_isdigit(fdname[0])) {
         qemu_error_new(QERR_INVALID_PARAMETER, "fdname");
-        return;
+        return -1;
     }
 
     fd = dup(fd);
@@ -2426,7 +2426,7 @@ static void do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
             qemu_error_new(QERR_TOO_MANY_FILES);
         else
             qemu_error_new(QERR_UNDEFINED_ERROR);
-        return;
+        return -1;
     }
 
     QLIST_FOREACH(monfd, &mon->fds, next) {
@@ -2436,7 +2436,7 @@ static void do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
         close(monfd->fd);
         monfd->fd = fd;
-        return;
+        return 0;
     }
 
     monfd = qemu_mallocz(sizeof(mon_fd_t));
@@ -2444,6 +2444,7 @@ static void do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
     monfd->fd = fd;
 
     QLIST_INSERT_HEAD(&mon->fds, monfd, next);
+    return 0;
 }
 
 static void do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index d2be5c4..e8e2ee1 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1083,7 +1083,7 @@ ETEXI
         .params     = "getfd name",
         .help       = "receive a file descriptor via SCM rights and assign it a name",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_getfd,
+        .cmd_new_ret = do_getfd,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 08/21] Monitor: Convert do_closefd() to cmd_new_ret()

[Luiz Capitulino]

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

diff --git a/monitor.c b/monitor.c
index 9466c63..9338d13 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2447,7 +2447,7 @@ static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return 0;
 }
 
-static void do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *fdname = qdict_get_str(qdict, "fdname");
     mon_fd_t *monfd;
@@ -2461,10 +2461,11 @@ static void do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
         close(monfd->fd);
         qemu_free(monfd->name);
         qemu_free(monfd);
-        return;
+        return 0;
     }
 
     qemu_error_new(QERR_FD_NOT_FOUND, fdname);
+    return -1;
 }
 
 static void do_loadvm(Monitor *mon, const QDict *qdict)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index e8e2ee1..6e84f76 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -1100,7 +1100,7 @@ ETEXI
         .params     = "closefd name",
         .help       = "close a file descriptor previously passed via SCM rights",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_closefd,
+        .cmd_new_ret = do_closefd,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 09/21] Monitor: Convert pci_device_hot_add() to cmd_new_ret()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/pci-hotplug.c |   16 +++++++++++-----
 qemu-monitor.hx  |    2 +-
 sysemu.h         |    2 +-
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 0fb96f0..6cc70d5 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -254,7 +254,7 @@ void pci_device_hot_add_print(Monitor *mon, const QObject *data)
  *
  * { "domain": 0, "bus": 0, "slot": 5, "function": 0 }
  */
-void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
+int pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     PCIDevice *dev = NULL;
     const char *pci_addr = qdict_get_str(qdict, "pci_addr");
@@ -273,20 +273,26 @@ void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (!strcmp(pci_addr, "auto"))
         pci_addr = NULL;
 
-    if (strcmp(type, "nic") == 0)
+    if (strcmp(type, "nic") == 0) {
         dev = qemu_pci_hot_add_nic(mon, pci_addr, opts);
-    else if (strcmp(type, "storage") == 0)
+    } else if (strcmp(type, "storage") == 0) {
         dev = qemu_pci_hot_add_storage(mon, pci_addr, opts);
-    else
+    } else {
         monitor_printf(mon, "invalid type: %s\n", type);
+        return -1;
+    }
 
     if (dev) {
         *ret_data =
         qobject_from_jsonf("{ 'domain': 0, 'bus': %d, 'slot': %d, "
                            "'function': %d }", pci_bus_num(dev->bus),
                            PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
-    } else
+    } else {
         monitor_printf(mon, "failed to add %s\n", opts);
+        return -1;
+    }
+
+    return 0;
 }
 #endif
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 6e84f76..2831055 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -857,7 +857,7 @@ ETEXI
         .params     = "auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...",
         .help       = "hot-add PCI device",
         .user_print = pci_device_hot_add_print,
-        .mhandler.cmd_new = pci_device_hot_add,
+        .cmd_new_ret = pci_device_hot_add,
     },
 #endif
 
diff --git a/sysemu.h b/sysemu.h
index 9c3b281..d41a338 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -213,7 +213,7 @@ DriveInfo *add_init_drive(const char *opts);
 
 /* pci-hotplug */
 void pci_device_hot_add_print(Monitor *mon, const QObject *data);
-void pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 void drive_hot_add(Monitor *mon, const QDict *qdict);
 void pci_device_hot_remove(Monitor *mon, const char *pci_addr);
 void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
-- 
1.6.6

[Qemu-devel] [PATCH 10/21] Monitor: Convert pci_device_hot_remove() to cmd_new_ret()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/pci-hotplug.c |   14 +++++++-------
 qemu-monitor.hx  |    2 +-
 sysemu.h         |    6 +++---
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 6cc70d5..bd82c6a 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -296,26 +296,26 @@ int pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
 }
 #endif
 
-void pci_device_hot_remove(Monitor *mon, const char *pci_addr)
+int pci_device_hot_remove(Monitor *mon, const char *pci_addr)
 {
     PCIDevice *d;
     int dom, bus;
     unsigned slot;
 
     if (pci_read_devaddr(mon, pci_addr, &dom, &bus, &slot)) {
-        return;
+        return -1;
     }
 
     d = pci_find_device(pci_find_root_bus(0), bus, slot, 0);
     if (!d) {
         monitor_printf(mon, "slot %d empty\n", slot);
-        return;
+        return -1;
     }
-    qdev_unplug(&d->qdev);
+    return qdev_unplug(&d->qdev);
 }
 
-void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
-                              QObject **ret_data)
+int do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
+                             QObject **ret_data)
 {
-    pci_device_hot_remove(mon, qdict_get_str(qdict, "pci_addr"));
+    return pci_device_hot_remove(mon, qdict_get_str(qdict, "pci_addr"));
 }
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 2831055..e987cf1 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -874,7 +874,7 @@ ETEXI
         .params     = "[[<domain>:]<bus>:]<slot>",
         .help       = "hot remove PCI device",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_pci_device_hot_remove,
+        .cmd_new_ret = do_pci_device_hot_remove,
     },
 #endif
 
diff --git a/sysemu.h b/sysemu.h
index d41a338..8ba618e 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -215,9 +215,9 @@ DriveInfo *add_init_drive(const char *opts);
 void pci_device_hot_add_print(Monitor *mon, const QObject *data);
 int pci_device_hot_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 void drive_hot_add(Monitor *mon, const QDict *qdict);
-void pci_device_hot_remove(Monitor *mon, const char *pci_addr);
-void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
-                              QObject **ret_data);
+int pci_device_hot_remove(Monitor *mon, const char *pci_addr);
+int do_pci_device_hot_remove(Monitor *mon, const QDict *qdict,
+                             QObject **ret_data);
 
 /* serial ports */
 
-- 
1.6.6

[Qemu-devel] [PATCH 11/21] Monitor: Convert do_migrate() to cmd_new_ret()

[Luiz Capitulino]

While there I'm also dropping a unneeded else clause (the last
one in the function).

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 migration.c     |   29 +++++++++++++++++------------
 migration.h     |    2 +-
 qemu-monitor.hx |    2 +-
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/migration.c b/migration.c
index 557bec4..05f6cc5 100644
--- a/migration.c
+++ b/migration.c
@@ -54,7 +54,7 @@ void qemu_start_incoming_migration(const char *uri)
         fprintf(stderr, "unknown migration protocol: %s\n", uri);
 }
 
-void do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
+int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     MigrationState *s = NULL;
     const char *p;
@@ -64,38 +64,43 @@ void do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (current_migration &&
         current_migration->get_status(current_migration) == MIG_STATE_ACTIVE) {
         monitor_printf(mon, "migration already in progress\n");
-        return;
+        return -1;
     }
 
-    if (strstart(uri, "tcp:", &p))
+    if (strstart(uri, "tcp:", &p)) {
         s = tcp_start_outgoing_migration(mon, p, max_throttle, detach,
                                          (int)qdict_get_int(qdict, "blk"), 
                                          (int)qdict_get_int(qdict, "inc"));
 #if !defined(WIN32)
-    else if (strstart(uri, "exec:", &p))
+    } else if (strstart(uri, "exec:", &p)) {
         s = exec_start_outgoing_migration(mon, p, max_throttle, detach,
                                           (int)qdict_get_int(qdict, "blk"), 
                                           (int)qdict_get_int(qdict, "inc"));
-    else if (strstart(uri, "unix:", &p))
+    } else if (strstart(uri, "unix:", &p)) {
         s = unix_start_outgoing_migration(mon, p, max_throttle, detach,
 					  (int)qdict_get_int(qdict, "blk"), 
                                           (int)qdict_get_int(qdict, "inc"));
-    else if (strstart(uri, "fd:", &p))
+    } else if (strstart(uri, "fd:", &p)) {
         s = fd_start_outgoing_migration(mon, p, max_throttle, detach, 
                                         (int)qdict_get_int(qdict, "blk"), 
                                         (int)qdict_get_int(qdict, "inc"));
 #endif
-    else
+    } else {
         monitor_printf(mon, "unknown migration protocol: %s\n", uri);
+        return -1;
+    }
 
-    if (s == NULL)
+    if (s == NULL) {
         monitor_printf(mon, "migration failed\n");
-    else {
-        if (current_migration)
-            current_migration->release(current_migration);
+        return -1;
+    }
 
-        current_migration = s;
+    if (current_migration) {
+        current_migration->release(current_migration);
     }
+
+    current_migration = s;
+    return 0;
 }
 
 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/migration.h b/migration.h
index 9345d97..385423f 100644
--- a/migration.h
+++ b/migration.h
@@ -52,7 +52,7 @@ struct FdMigrationState
 
 void qemu_start_incoming_migration(const char *uri);
 
-void do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 int do_migrate_cancel(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index e987cf1..3a1f9bc 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -773,7 +773,7 @@ ETEXI
 		      "shared storage with incremental copy of disk "
 		      "(base image shared between src and destination)",
         .user_print = monitor_user_noop,	
-	.mhandler.cmd_new = do_migrate,
+	.cmd_new_ret = do_migrate,
     },
 
 
-- 
1.6.6

[Qemu-devel] [PATCH 12/21] Monitor: Convert do_memory_save() to cmd_new_ret()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    9 +++++++--
 qemu-monitor.hx |    2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9338d13..7747449 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1428,7 +1428,7 @@ static void do_print(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "\n");
 }
 
-static void do_memory_save(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_memory_save(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     FILE *f;
     uint32_t size = qdict_get_int(qdict, "size");
@@ -1437,13 +1437,14 @@ static void do_memory_save(Monitor *mon, const QDict *qdict, QObject **ret_data)
     uint32_t l;
     CPUState *env;
     uint8_t buf[1024];
+    int ret = -1;
 
     env = mon_get_cpu();
 
     f = fopen(filename, "wb");
     if (!f) {
         qemu_error_new(QERR_OPEN_FILE_FAILED, filename);
-        return;
+        return -1;
     }
     while (size != 0) {
         l = sizeof(buf);
@@ -1457,8 +1458,12 @@ static void do_memory_save(Monitor *mon, const QDict *qdict, QObject **ret_data)
         addr += l;
         size -= l;
     }
+
+    ret = 0;
+
 exit:
     fclose(f);
+    return ret;
 }
 
 static void do_physical_memory_save(Monitor *mon, const QDict *qdict,
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 3a1f9bc..5995cef 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -705,7 +705,7 @@ ETEXI
         .params     = "addr size file",
         .help       = "save to disk virtual memory dump starting at 'addr' of size 'size'",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_memory_save,
+        .cmd_new_ret = do_memory_save,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 13/21] Monitor: Convert do_physical_memory_save() to cmd_new_ret()

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    9 +++++++--
 qemu-monitor.hx |    2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 7747449..3470b0b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1466,7 +1466,7 @@ exit:
     return ret;
 }
 
-static void do_physical_memory_save(Monitor *mon, const QDict *qdict,
+static int do_physical_memory_save(Monitor *mon, const QDict *qdict,
                                     QObject **ret_data)
 {
     FILE *f;
@@ -1475,11 +1475,12 @@ static void do_physical_memory_save(Monitor *mon, const QDict *qdict,
     uint32_t size = qdict_get_int(qdict, "size");
     const char *filename = qdict_get_str(qdict, "filename");
     target_phys_addr_t addr = qdict_get_int(qdict, "val");
+    int ret = -1;
 
     f = fopen(filename, "wb");
     if (!f) {
         qemu_error_new(QERR_OPEN_FILE_FAILED, filename);
-        return;
+        return -1;
     }
     while (size != 0) {
         l = sizeof(buf);
@@ -1494,8 +1495,12 @@ static void do_physical_memory_save(Monitor *mon, const QDict *qdict,
         addr += l;
         size -= l;
     }
+
+    ret = 0;
+
 exit:
     fclose(f);
+    return ret;
 }
 
 static void do_sum(Monitor *mon, const QDict *qdict)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 5995cef..5ae1969 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -720,7 +720,7 @@ ETEXI
         .params     = "addr size file",
         .help       = "save to disk physical memory dump starting at 'addr' of size 'size'",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_physical_memory_save,
+        .cmd_new_ret = do_physical_memory_save,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 14/21] Monitor: Convert do_info() to cmd_new_ret()

[Luiz Capitulino]

Note that this function only fails in QMP, in the user Monitor
it prints the help text instead.

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

diff --git a/monitor.c b/monitor.c
index 3470b0b..baedc34 100644
--- a/monitor.c
+++ b/monitor.c
@@ -556,7 +556,7 @@ static void user_async_info_handler(Monitor *mon, const mon_cmd_t *cmd)
     }
 }
 
-static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const mon_cmd_t *cmd;
     const char *item = qdict_get_try_str(qdict, "item");
@@ -574,7 +574,7 @@ static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
     if (cmd->name == NULL) {
         if (monitor_ctrl_mode(mon)) {
             qemu_error_new(QERR_COMMAND_NOT_FOUND, item);
-            return;
+            return -1;
         }
         goto help;
     }
@@ -606,15 +606,17 @@ static void do_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
         if (monitor_ctrl_mode(mon)) {
             /* handler not converted yet */
             qemu_error_new(QERR_COMMAND_NOT_FOUND, item);
+            return -1;
         } else {
             cmd->mhandler.info(mon);
         }
     }
 
-    return;
+    return 0;
 
 help:
     help_cmd(mon, "info");
+    return 0;
 }
 
 static void do_info_version_print(Monitor *mon, const QObject *data)
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 5ae1969..7bfb8d0 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -43,7 +43,7 @@ ETEXI
         .params     = "[subcommand]",
         .help       = "show various information about the system state",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_info,
+        .cmd_new_ret = do_info,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 15/21] Monitor: Convert do_change() to cmd_new_ret()

[Luiz Capitulino]

Not that trivial as the call chain also has to be modified.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |   60 ++++++++++++++++++++++++++++++++++--------------------
 monitor.h       |    6 ++--
 qemu-monitor.hx |    2 +-
 3 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/monitor.c b/monitor.c
index baedc34..37a2a48 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1027,8 +1027,8 @@ static int do_block_set_passwd(Monitor *mon, const QDict *qdict,
     return 0;
 }
 
-static void do_change_block(Monitor *mon, const char *device,
-                            const char *filename, const char *fmt)
+static int do_change_block(Monitor *mon, const char *device,
+                           const char *filename, const char *fmt)
 {
     BlockDriverState *bs;
     BlockDriver *drv = NULL;
@@ -1036,26 +1036,32 @@ static void do_change_block(Monitor *mon, const char *device,
     bs = bdrv_find(device);
     if (!bs) {
         qemu_error_new(QERR_DEVICE_NOT_FOUND, device);
-        return;
+        return -1;
     }
     if (fmt) {
         drv = bdrv_find_whitelisted_format(fmt);
         if (!drv) {
             qemu_error_new(QERR_INVALID_BLOCK_FORMAT, fmt);
-            return;
+            return -1;
         }
     }
-    if (eject_device(mon, bs, 0) < 0)
-        return;
-    bdrv_open2(bs, filename, BDRV_O_RDWR, drv);
-    monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
+    if (eject_device(mon, bs, 0) < 0) {
+        return -1;
+    }
+    if (bdrv_open2(bs, filename, BDRV_O_RDWR, drv) < 0) {
+        return -1;
+    }
+    return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 
-static void change_vnc_password(const char *password)
+static int change_vnc_password(const char *password)
 {
-    if (vnc_display_password(NULL, password) < 0)
+    if (vnc_display_password(NULL, password) < 0) {
         qemu_error_new(QERR_SET_PASSWD_FAILED);
+        return -1;
+    }
 
+    return 0;
 }
 
 static void change_vnc_password_cb(Monitor *mon, const char *password,
@@ -1065,7 +1071,7 @@ static void change_vnc_password_cb(Monitor *mon, const char *password,
     monitor_read_command(mon, 1);
 }
 
-static void do_change_vnc(Monitor *mon, const char *target, const char *arg)
+static int do_change_vnc(Monitor *mon, const char *target, const char *arg)
 {
     if (strcmp(target, "passwd") == 0 ||
         strcmp(target, "password") == 0) {
@@ -1073,29 +1079,37 @@ static void do_change_vnc(Monitor *mon, const char *target, const char *arg)
             char password[9];
             strncpy(password, arg, sizeof(password));
             password[sizeof(password) - 1] = '\0';
-            change_vnc_password(password);
+            return change_vnc_password(password);
         } else {
-            monitor_read_password(mon, change_vnc_password_cb, NULL);
+            return monitor_read_password(mon, change_vnc_password_cb, NULL);
         }
     } else {
-        if (vnc_display_open(NULL, target) < 0)
+        if (vnc_display_open(NULL, target) < 0) {
             qemu_error_new(QERR_VNC_SERVER_FAILED, target);
+            return -1;
+        }
     }
+
+    return 0;
 }
 
 /**
  * do_change(): Change a removable medium, or VNC configuration
  */
-static void do_change(Monitor *mon, const QDict *qdict, QObject **ret_data)
+static int do_change(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *device = qdict_get_str(qdict, "device");
     const char *target = qdict_get_str(qdict, "target");
     const char *arg = qdict_get_try_str(qdict, "arg");
+    int ret;
+
     if (strcmp(device, "vnc") == 0) {
-        do_change_vnc(mon, target, arg);
+        ret = do_change_vnc(mon, target, arg);
     } else {
-        do_change_block(mon, device, target, arg);
+        ret = do_change_block(mon, device, target, arg);
     }
+
+    return ret;
 }
 
 static void do_screen_dump(Monitor *mon, const QDict *qdict)
@@ -4554,21 +4568,21 @@ static void bdrv_password_cb(Monitor *mon, const char *password, void *opaque)
     monitor_read_command(mon, 1);
 }
 
-void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-                                 BlockDriverCompletionFunc *completion_cb,
-                                 void *opaque)
+int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
+                                BlockDriverCompletionFunc *completion_cb,
+                                void *opaque)
 {
     int err;
 
     if (!bdrv_key_required(bs)) {
         if (completion_cb)
             completion_cb(opaque, 0);
-        return;
+        return 0;
     }
 
     if (monitor_ctrl_mode(mon)) {
         qemu_error_new(QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs));
-        return;
+        return -1;
     }
 
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
@@ -4581,6 +4595,8 @@ void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
 
     if (err && completion_cb)
         completion_cb(opaque, err);
+
+    return err;
 }
 
 typedef struct QemuErrorSink QemuErrorSink;
diff --git a/monitor.h b/monitor.h
index e35f1e4..fc09505 100644
--- a/monitor.h
+++ b/monitor.h
@@ -33,9 +33,9 @@ void monitor_init(CharDriverState *chr, int flags);
 int monitor_suspend(Monitor *mon);
 void monitor_resume(Monitor *mon);
 
-void monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-                                 BlockDriverCompletionFunc *completion_cb,
-                                 void *opaque);
+int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
+                                BlockDriverCompletionFunc *completion_cb,
+                                void *opaque);
 
 int monitor_get_fd(Monitor *mon, const char *fdname);
 
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 7bfb8d0..d5b344e 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -153,7 +153,7 @@ ETEXI
         .params     = "device filename [format]",
         .help       = "change a removable medium, optional format",
         .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_change,
+        .cmd_new_ret = do_change,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 16/21] Monitor: Rename cmd_new_ret()

[Luiz Capitulino]

Now that all handlers are converted to cmd_new_ret(), we can rename
it back to cmd_new(). But now it returns a value.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 monitor.c       |    9 ++-------
 qemu-monitor.hx |   42 +++++++++++++++++++++---------------------
 2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/monitor.c b/monitor.c
index 37a2a48..6eb0e2c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -98,13 +98,12 @@ typedef struct mon_cmd_t {
     const char *params;
     const char *help;
     void (*user_print)(Monitor *mon, const QObject *data);
-    int (*cmd_new_ret)(Monitor *mon, const QDict *params, QObject **ret_data);
     union {
         void (*info)(Monitor *mon);
         void (*info_new)(Monitor *mon, QObject **ret_data);
         int  (*info_async)(Monitor *mon, MonitorCompletion *cb, void *opaque);
         void (*cmd)(Monitor *mon, const QDict *qdict);
-        void (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
+        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
         int  (*cmd_async)(Monitor *mon, const QDict *params,
                           MonitorCompletion *cb, void *opaque);
     } mhandler;
@@ -3846,11 +3845,7 @@ static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
 {
     QObject *data = NULL;
 
-    if (cmd->cmd_new_ret) {
-        cmd->cmd_new_ret(mon, params, &data);
-    } else {
-        cmd->mhandler.cmd_new(mon, params, &data);
-    }
+    cmd->mhandler.cmd_new(mon, params, &data);
 
     if (is_async_return(data)) {
         /*
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index d5b344e..7f9d261 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -43,7 +43,7 @@ ETEXI
         .params     = "[subcommand]",
         .help       = "show various information about the system state",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_info,
+        .mhandler.cmd_new = do_info,
     },
 
 STEXI
@@ -123,7 +123,7 @@ ETEXI
         .params     = "",
         .help       = "quit the emulator",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_quit,
+        .mhandler.cmd_new = do_quit,
     },
 
 STEXI
@@ -138,7 +138,7 @@ ETEXI
         .params     = "[-f] device",
         .help       = "eject a removable medium (use -f to force it)",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_eject,
+        .mhandler.cmd_new = do_eject,
     },
 
 STEXI
@@ -153,7 +153,7 @@ ETEXI
         .params     = "device filename [format]",
         .help       = "change a removable medium, optional format",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_change,
+        .mhandler.cmd_new = do_change,
     },
 
 STEXI
@@ -303,7 +303,7 @@ ETEXI
         .params     = "",
         .help       = "stop emulation",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_stop,
+        .mhandler.cmd_new = do_stop,
     },
 
 STEXI
@@ -318,7 +318,7 @@ ETEXI
         .params     = "",
         .help       = "resume emulation",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_cont,
+        .mhandler.cmd_new = do_cont,
     },
 
 STEXI
@@ -494,7 +494,7 @@ ETEXI
         .params     = "",
         .help       = "reset the system",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_system_reset,
+        .mhandler.cmd_new = do_system_reset,
     },
 
 STEXI
@@ -510,7 +510,7 @@ ETEXI
         .params     = "",
         .help       = "send system power down event",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_system_powerdown,
+        .mhandler.cmd_new = do_system_powerdown,
     },
 
 STEXI
@@ -604,7 +604,7 @@ ETEXI
         .params     = "index",
         .help       = "set the default CPU",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_cpu_set,
+        .mhandler.cmd_new = do_cpu_set,
     },
 
 STEXI
@@ -705,7 +705,7 @@ ETEXI
         .params     = "addr size file",
         .help       = "save to disk virtual memory dump starting at 'addr' of size 'size'",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_memory_save,
+        .mhandler.cmd_new = do_memory_save,
     },
 
 STEXI
@@ -720,7 +720,7 @@ ETEXI
         .params     = "addr size file",
         .help       = "save to disk physical memory dump starting at 'addr' of size 'size'",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_physical_memory_save,
+        .mhandler.cmd_new = do_physical_memory_save,
     },
 
 STEXI
@@ -773,7 +773,7 @@ ETEXI
 		      "shared storage with incremental copy of disk "
 		      "(base image shared between src and destination)",
         .user_print = monitor_user_noop,	
-	.cmd_new_ret = do_migrate,
+	.mhandler.cmd_new = do_migrate,
     },
 
 
@@ -791,7 +791,7 @@ ETEXI
         .params     = "",
         .help       = "cancel the current VM migration",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_migrate_cancel,
+        .mhandler.cmd_new = do_migrate_cancel,
     },
 
 STEXI
@@ -806,7 +806,7 @@ ETEXI
         .params     = "value",
         .help       = "set maximum speed (in bytes) for migrations",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_migrate_set_speed,
+        .mhandler.cmd_new = do_migrate_set_speed,
     },
 
 STEXI
@@ -821,7 +821,7 @@ ETEXI
         .params     = "value",
         .help       = "set maximum tolerated downtime (in seconds) for migrations",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_migrate_set_downtime,
+        .mhandler.cmd_new = do_migrate_set_downtime,
     },
 
 STEXI
@@ -857,7 +857,7 @@ ETEXI
         .params     = "auto|[[<domain>:]<bus>:]<slot> nic|storage [[vlan=n][,macaddr=addr][,model=type]] [file=file][,if=type][,bus=nr]...",
         .help       = "hot-add PCI device",
         .user_print = pci_device_hot_add_print,
-        .cmd_new_ret = pci_device_hot_add,
+        .mhandler.cmd_new = pci_device_hot_add,
     },
 #endif
 
@@ -874,7 +874,7 @@ ETEXI
         .params     = "[[<domain>:]<bus>:]<slot>",
         .help       = "hot remove PCI device",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_pci_device_hot_remove,
+        .mhandler.cmd_new = do_pci_device_hot_remove,
     },
 #endif
 
@@ -1083,7 +1083,7 @@ ETEXI
         .params     = "getfd name",
         .help       = "receive a file descriptor via SCM rights and assign it a name",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_getfd,
+        .mhandler.cmd_new = do_getfd,
     },
 
 STEXI
@@ -1100,7 +1100,7 @@ ETEXI
         .params     = "closefd name",
         .help       = "close a file descriptor previously passed via SCM rights",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_closefd,
+        .mhandler.cmd_new = do_closefd,
     },
 
 STEXI
@@ -1117,7 +1117,7 @@ ETEXI
         .params     = "block_passwd device password",
         .help       = "set the password of encrypted block devices",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_block_set_passwd,
+        .mhandler.cmd_new = do_block_set_passwd,
     },
 
 STEXI
@@ -1132,7 +1132,7 @@ ETEXI
         .params     = "",
         .help       = "enable QMP capabilities",
         .user_print = monitor_user_noop,
-        .cmd_new_ret = do_qmp_capabilities,
+        .mhandler.cmd_new = do_qmp_capabilities,
     },
 
 STEXI
-- 
1.6.6

[Qemu-devel] [PATCH 17/21] Monitor: Debugging support

[Luiz Capitulino]

Add configure options (--enable-debug-mon and --disable-debug-mon)
plus the MON_DEBUG() macro.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 configure |   10 ++++++++++
 monitor.c |    8 ++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/configure b/configure
index 0a84b0e..2c2e7ae 100755
--- a/configure
+++ b/configure
@@ -266,6 +266,7 @@ linux_aio=""
 
 gprof="no"
 debug_tcg="no"
+debug_mon="no"
 debug="no"
 strip_opt="yes"
 bigendian="no"
@@ -514,9 +515,14 @@ for opt do
   ;;
   --disable-debug-tcg) debug_tcg="no"
   ;;
+  --enable-debug-mon) debug_mon="yes"
+  ;;
+  --disable-debug-mon) debug_mon="no"
+  ;;
   --enable-debug)
       # Enable debugging options that aren't excessively noisy
       debug_tcg="yes"
+      debug_mon="yes"
       debug="yes"
       strip_opt="no"
   ;;
@@ -1918,6 +1924,7 @@ echo "host CPU          $cpu"
 echo "host big endian   $bigendian"
 echo "target list       $target_list"
 echo "tcg debug enabled $debug_tcg"
+echo "Mon debug enabled $debug_mon"
 echo "gprof enabled     $gprof"
 echo "sparse enabled    $sparse"
 echo "strip binaries    $strip_opt"
@@ -1995,6 +2002,9 @@ echo "ARCH=$ARCH" >> $config_host_mak
 if test "$debug_tcg" = "yes" ; then
   echo "CONFIG_DEBUG_TCG=y" >> $config_host_mak
 fi
+if test "$debug_mon" = "yes" ; then
+  echo "CONFIG_DEBUG_MONITOR=y" >> $config_host_mak
+fi
 if test "$debug" = "yes" ; then
   echo "CONFIG_DEBUG_EXEC=y" >> $config_host_mak
 fi
diff --git a/monitor.c b/monitor.c
index 6eb0e2c..2a14e81 100644
--- a/monitor.c
+++ b/monitor.c
@@ -143,6 +143,14 @@ struct Monitor {
     QLIST_ENTRY(Monitor) entry;
 };
 
+#ifdef CONFIG_DEBUG_MONITOR
+#define MON_DEBUG(fmt, ...) do {    \
+    fprintf(stderr, "Monitor: ");       \
+    fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+#else /* !CONFIG_DEBUG_MONITOR */
+#define MON_DEBUG(fmt, ...) do { } while (0)
+#endif /* CONFIG_DEBUG_MONITOR */
+
 static QLIST_HEAD(mon_list, Monitor) mon_list;
 
 static const mon_cmd_t mon_cmds[];
-- 
1.6.6

[Qemu-devel] [PATCH 18/21] Monitor: Drop the print disabling mechanism

[Luiz Capitulino]

We can ignore calls to monitor_vprintf() in QMP mode and use
monitor_puts() directly in monitor_json_emitter().

This allows us to drop this ugly hack.

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

diff --git a/monitor.c b/monitor.c
index 2a14e81..d5b406c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -120,7 +120,6 @@ struct mon_fd_t {
 
 typedef struct MonitorControl {
     QObject *id;
-    int print_enabled;
     JSONMessageParser parser;
     int command_mode;
 } MonitorControl;
@@ -226,16 +225,18 @@ static void monitor_puts(Monitor *mon, const char *str)
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
+    char buf[4096];
+
     if (!mon)
         return;
 
-    if (mon->mc && !mon->mc->print_enabled) {
+    if (monitor_ctrl_mode(mon)) {
         qemu_error_new(QERR_UNDEFINED_ERROR);
-    } else {
-        char buf[4096];
-        vsnprintf(buf, sizeof(buf), fmt, ap);
-        monitor_puts(mon, buf);
+        return;
     }
+
+    vsnprintf(buf, sizeof(buf), fmt, ap);
+    monitor_puts(mon, buf);
 }
 
 void monitor_printf(Monitor *mon, const char *fmt, ...)
@@ -306,9 +307,8 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)
     json = qobject_to_json(data);
     assert(json != NULL);
 
-    mon->mc->print_enabled = 1;
-    monitor_printf(mon, "%s\n", qstring_get_str(json));
-    mon->mc->print_enabled = 0;
+    qstring_append_chr(json, '\n');
+    monitor_puts(mon, qstring_get_str(json));
 
     QDECREF(json);
 }
-- 
1.6.6

[Qemu-devel] [PATCH 19/21] Monitor: Audit handler return

[Luiz Capitulino]

This commit verifies the following two rules specified by
Markus Armbruster:

1. If the handler returns failure, it must have passed an error.

   If it didn't, it's broken. Report an internal error to the client,
   and report the bug to the programmer.

2. If the handler returns success, it must not have passed an error.

   If it did, it's broken. Report the error to the client, and report
   the bug to the programmer.

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

diff --git a/monitor.c b/monitor.c
index d5b406c..2f43136 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3848,12 +3848,42 @@ static int is_async_return(const QObject *data)
     return 0;
 }
 
+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.
+         */
+        if (monitor_ctrl_mode(mon)) {
+            qemu_error_new(QERR_UNDEFINED_ERROR);
+        }
+        MON_DEBUG("command '%s' returned failure but did not pass an error\n",
+                  cmd->name);
+    }
+
+#ifdef CONFIG_DEBUG_MONITOR
+    if (!ret && monitor_has_error(mon)) {
+        /*
+         * If it returns success, it must not have passed an error.
+         *
+         * Action: Report the passed error to the client.
+         */
+        MON_DEBUG("command '%s' returned success but passed an error\n",
+                  cmd->name);
+    }
+#endif
+}
+
 static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
                                  const QDict *params)
 {
+    int ret;
     QObject *data = NULL;
 
-    cmd->mhandler.cmd_new(mon, params, &data);
+    ret = cmd->mhandler.cmd_new(mon, params, &data);
+    handler_audit(mon, cmd, ret);
 
     if (is_async_return(data)) {
         /*
-- 
1.6.6

[Qemu-devel] [PATCH 20/21] Monitor: Debug stray prints the right way

[Luiz Capitulino]

QObject Monitor handlers should not call any Monitor print
function: they should only build objects, printing is done
by common code.

Current QMP code will ignore such calls, as we can't send
garbage to clients, additionally it will also emit an
undefined error on the assumption that print calls usually
report errors.

However, the right way to deal with this is to rely on a
return code. This has been fixed by other commit already.

Now, this commit drops the error from monitor_vprintf() and
adds a better debugging mechanism for those 'stray' prints:
we count them if debug is enabled and let the developer know
if a QObject handler is trying to print anything.

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

diff --git a/monitor.c b/monitor.c
index 2f43136..d9592e9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -137,6 +137,9 @@ struct Monitor {
     CPUState *mon_cpu;
     BlockDriverCompletionFunc *password_completion_cb;
     void *password_opaque;
+#ifdef CONFIG_DEBUG_MONITOR
+    int print_calls_nr;
+#endif
     QError *error;
     QLIST_HEAD(,mon_fd_t) fds;
     QLIST_ENTRY(Monitor) entry;
@@ -146,8 +149,27 @@ struct Monitor {
 #define MON_DEBUG(fmt, ...) do {    \
     fprintf(stderr, "Monitor: ");       \
     fprintf(stderr, fmt, ## __VA_ARGS__); } while (0)
+
+static inline void mon_print_count_inc(Monitor *mon)
+{
+    mon->print_calls_nr++;
+}
+
+static inline void mon_print_count_init(Monitor *mon)
+{
+    mon->print_calls_nr = 0;
+}
+
+static inline int mon_print_count_get(const Monitor *mon)
+{
+    return mon->print_calls_nr;
+}
+
 #else /* !CONFIG_DEBUG_MONITOR */
 #define MON_DEBUG(fmt, ...) do { } while (0)
+static inline void mon_print_count_inc(Monitor *mon) { }
+static inline void mon_print_count_init(Monitor *mon) { }
+static inline int mon_print_count_get(const Monitor *mon) { return 0; }
 #endif /* CONFIG_DEBUG_MONITOR */
 
 static QLIST_HEAD(mon_list, Monitor) mon_list;
@@ -230,8 +252,9 @@ void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     if (!mon)
         return;
 
+    mon_print_count_inc(mon);
+
     if (monitor_ctrl_mode(mon)) {
-        qemu_error_new(QERR_UNDEFINED_ERROR);
         return;
     }
 
@@ -3873,6 +3896,21 @@ static void handler_audit(Monitor *mon, const mon_cmd_t *cmd, int ret)
         MON_DEBUG("command '%s' returned success but passed an error\n",
                   cmd->name);
     }
+
+    if (mon_print_count_get(mon) > 0 && strcmp(cmd->name, "info") != 0) {
+        /*
+         * Handlers should not call Monitor print functions.
+         *
+         * Action: Ignore them in QMP.
+         *
+         * (XXX: we don't check any 'info' or 'query' command here
+         * because the user print function _is_ called by do_info(), hence
+         * we will trigger this check. This problem will go away when we
+         * make 'query' commands real and kill do_info())
+         */
+        MON_DEBUG("command '%s' called print functions %d time(s)\n",
+                  cmd->name, mon_print_count_get(mon));
+    }
 #endif
 }
 
@@ -3882,6 +3920,8 @@ static void monitor_call_handler(Monitor *mon, const mon_cmd_t *cmd,
     int ret;
     QObject *data = NULL;
 
+    mon_print_count_init(mon);
+
     ret = cmd->mhandler.cmd_new(mon, params, &data);
     handler_audit(mon, cmd, ret);
 
-- 
1.6.6

[Qemu-devel] [PATCH 21/21] Monitor: Report more than one error in handlers

[Luiz Capitulino]

Handlers can generate only one error in a call, we let the
programmer know if they brake this rule and clients will only
get the first generated error.

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

diff --git a/monitor.c b/monitor.c
index d9592e9..6a64e6e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4760,7 +4760,8 @@ void qemu_error_internal(const char *file, int linenr, const char *func,
         if (!qemu_error_sink->mon->error) {
             qemu_error_sink->mon->error = qerror;
         } else {
-            /* XXX: warn the programmer */
+            MON_DEBUG("Additional error report at %s:%d\n", qerror->file,
+                      qerror->linenr);
             QDECREF(qerror);
         }
         break;
-- 
1.6.6

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Markus Armbruster]

Excellent job!  I'll base my next patch submissions on this series,
because that way I can resolve the conflicts now rather than after
Anthony bounced my patches right back to me.  Anthony, merging this
sooner rather than later would help me.  No need for undue haste, of
course.

There are a few opportunities for cleanup after insertion of return
(stuff like "return -1; } else {"), but that's best done separately, to
keep the conversion obvious.

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Luiz Capitulino]

On Thu, 11 Feb 2010 09:58:43 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Excellent job!  I'll base my next patch submissions on this series,
> because that way I can resolve the conflicts now rather than after
> Anthony bounced my patches right back to me.  Anthony, merging this
> sooner rather than later would help me.  No need for undue haste, of
> course.

 Hope you have reviewed in detail too :)

> There are a few opportunities for cleanup after insertion of return
> (stuff like "return -1; } else {"), but that's best done separately, to
> keep the conversion obvious.

 Agreed.

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 11 Feb 2010 09:58:43 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Excellent job!  I'll base my next patch submissions on this series,
>> because that way I can resolve the conflicts now rather than after
>> Anthony bounced my patches right back to me.  Anthony, merging this
>> sooner rather than later would help me.  No need for undue haste, of
>> course.
>
>  Hope you have reviewed in detail too :)

Yup, did that.

[...]

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Anthony Liguori]

On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
>   Hi there,
>
>   When I started converting handlers to the QObject style, I thought that
> returning an error code wouldn't be needed. That is, we have an error object
> already, so if the handler returns the error object it has failed, otherwise
> it has succeeded.
>
>   This was also very convenient, because handlers have never returned an error
> code, and thus it would be easier to add a call to qemu_error_new() in the
> right place instead of returning error codes.
>
>   Turns out we need both. Actually, I should not have abused the error object
> this way because (as Markus says) this is too fragile and we can end up
> reporting bogus errors to clients (among other problems).
>
>   The good news is that it's easy to fix.
>
>   All we have to do is to change cmd_new() (the handler callback) to return an
> error code and convert existing QObject handlers to it. This series does that
> and most of the patches are really straightforward conversions.
>
>   Additionally, Markus has designed an excellent debug mechanism for QMP, which
> is implemented by the last patches in this series, and will allow us to catch
> important QObject conversion and error handling bugs in handlers.
>    

Instead of returning -1, would it make more sense to return an error 
object?  If fact, why not drop ret_data as a passed in parameter, and 
just always return either the result or an error object.

Regards,

Anthony Liguori

>   Thanks.
>
>
>
>    

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
>>   Hi there,
>>
>>   When I started converting handlers to the QObject style, I thought that
>> returning an error code wouldn't be needed. That is, we have an error object
>> already, so if the handler returns the error object it has failed, otherwise
>> it has succeeded.
>>
>>   This was also very convenient, because handlers have never returned an error
>> code, and thus it would be easier to add a call to qemu_error_new() in the
>> right place instead of returning error codes.
>>
>>   Turns out we need both. Actually, I should not have abused the error object
>> this way because (as Markus says) this is too fragile and we can end up
>> reporting bogus errors to clients (among other problems).
>>
>>   The good news is that it's easy to fix.
>>
>>   All we have to do is to change cmd_new() (the handler callback) to return an
>> error code and convert existing QObject handlers to it. This series does that
>> and most of the patches are really straightforward conversions.
>>
>>   Additionally, Markus has designed an excellent debug mechanism for QMP, which
>> is implemented by the last patches in this series, and will allow us to catch
>> important QObject conversion and error handling bugs in handlers.
>>    
>
> Instead of returning -1, would it make more sense to return an error
> object?  If fact, why not drop ret_data as a passed in parameter, and
> just always return either the result or an error object.

Tempting, isn't it?

The practical trouble with this idea is that you have to adjust a lot of
code to return error objects all the way up into the handler.  With the
current design, you emit error objects "sideways", into the monitor
state.  This lets us keep the current mechanisms to report success /
failure (return >= 0 / -1; non-NULL / NULL, non-zero / zero, ...).

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Luiz Capitulino]

On Thu, 11 Feb 2010 16:27:00 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
> >>   Hi there,
> >>
> >>   When I started converting handlers to the QObject style, I thought that
> >> returning an error code wouldn't be needed. That is, we have an error object
> >> already, so if the handler returns the error object it has failed, otherwise
> >> it has succeeded.
> >>
> >>   This was also very convenient, because handlers have never returned an error
> >> code, and thus it would be easier to add a call to qemu_error_new() in the
> >> right place instead of returning error codes.
> >>
> >>   Turns out we need both. Actually, I should not have abused the error object
> >> this way because (as Markus says) this is too fragile and we can end up
> >> reporting bogus errors to clients (among other problems).
> >>
> >>   The good news is that it's easy to fix.
> >>
> >>   All we have to do is to change cmd_new() (the handler callback) to return an
> >> error code and convert existing QObject handlers to it. This series does that
> >> and most of the patches are really straightforward conversions.
> >>
> >>   Additionally, Markus has designed an excellent debug mechanism for QMP, which
> >> is implemented by the last patches in this series, and will allow us to catch
> >> important QObject conversion and error handling bugs in handlers.
> >>    
> >
> > Instead of returning -1, would it make more sense to return an error
> > object?  If fact, why not drop ret_data as a passed in parameter, and
> > just always return either the result or an error object.
> 
> Tempting, isn't it?
> 
> The practical trouble with this idea is that you have to adjust a lot of
> code to return error objects all the way up into the handler.  With the
> current design, you emit error objects "sideways", into the monitor
> state.  This lets us keep the current mechanisms to report success /
> failure (return >= 0 / -1; non-NULL / NULL, non-zero / zero, ...).

 Yes, also note that some functions are used by other subsystems. If they
return -1/0 we'd have to adjust other call chains as well.

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Anthony Liguori]

On 02/11/2010 09:27 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>
>    
>> On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
>>      
>>>    Hi there,
>>>
>>>    When I started converting handlers to the QObject style, I thought that
>>> returning an error code wouldn't be needed. That is, we have an error object
>>> already, so if the handler returns the error object it has failed, otherwise
>>> it has succeeded.
>>>
>>>    This was also very convenient, because handlers have never returned an error
>>> code, and thus it would be easier to add a call to qemu_error_new() in the
>>> right place instead of returning error codes.
>>>
>>>    Turns out we need both. Actually, I should not have abused the error object
>>> this way because (as Markus says) this is too fragile and we can end up
>>> reporting bogus errors to clients (among other problems).
>>>
>>>    The good news is that it's easy to fix.
>>>
>>>    All we have to do is to change cmd_new() (the handler callback) to return an
>>> error code and convert existing QObject handlers to it. This series does that
>>> and most of the patches are really straightforward conversions.
>>>
>>>    Additionally, Markus has designed an excellent debug mechanism for QMP, which
>>> is implemented by the last patches in this series, and will allow us to catch
>>> important QObject conversion and error handling bugs in handlers.
>>>
>>>        
>> Instead of returning -1, would it make more sense to return an error
>> object?  If fact, why not drop ret_data as a passed in parameter, and
>> just always return either the result or an error object.
>>      
> Tempting, isn't it?
>
> The practical trouble with this idea is that you have to adjust a lot of
> code to return error objects all the way up into the handler.  With the
> current design, you emit error objects "sideways",

But you still have to propagate an error return somewhere in order to 
short circuit the execution of the handler.

So you end up doing:

func1:
   qerror_new();
   return -1;

func2:
   err = func1();
   if (err == -1)
     return -1;

func3:
   err = func2();
   if (err == -1)
     return -1;

Would it be just as easy to say:

func1:
   return qerror_new();

func2:
   obj = func1();
   if qobject_is_qerror(obj):
      return obj;

func3:
   obj = func2();
   if qobject_is_qerror(obj):
     return obj;

Ultimately, this you the ability to decide in func3 whether you want to 
continue propagating the error or whether you want to take corrective 
action.

The current qerror stuff is really only useful within a single 
function.  Once you start building infrastructure around qerror, it 
becomes very difficult to deal with.

Regards,

Anthony Liguori

>   into the monitor
> state.  This lets us keep the current mechanisms to report success /
> failure (return>= 0 / -1; non-NULL / NULL, non-zero / zero, ...).
>    

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/11/2010 09:27 AM, Markus Armbruster wrote:
>> Anthony Liguori<anthony@codemonkey.ws>  writes:
>>
>>    
>>> On 02/10/2010 07:49 PM, Luiz Capitulino wrote:
>>>      
>>>>    Hi there,
>>>>
>>>>    When I started converting handlers to the QObject style, I thought that
>>>> returning an error code wouldn't be needed. That is, we have an error object
>>>> already, so if the handler returns the error object it has failed, otherwise
>>>> it has succeeded.
>>>>
>>>>    This was also very convenient, because handlers have never returned an error
>>>> code, and thus it would be easier to add a call to qemu_error_new() in the
>>>> right place instead of returning error codes.
>>>>
>>>>    Turns out we need both. Actually, I should not have abused the error object
>>>> this way because (as Markus says) this is too fragile and we can end up
>>>> reporting bogus errors to clients (among other problems).
>>>>
>>>>    The good news is that it's easy to fix.
>>>>
>>>>    All we have to do is to change cmd_new() (the handler callback) to return an
>>>> error code and convert existing QObject handlers to it. This series does that
>>>> and most of the patches are really straightforward conversions.
>>>>
>>>>    Additionally, Markus has designed an excellent debug mechanism for QMP, which
>>>> is implemented by the last patches in this series, and will allow us to catch
>>>> important QObject conversion and error handling bugs in handlers.
>>>>
>>>>        
>>> Instead of returning -1, would it make more sense to return an error
>>> object?  If fact, why not drop ret_data as a passed in parameter, and
>>> just always return either the result or an error object.
>>>      
>> Tempting, isn't it?
>>
>> The practical trouble with this idea is that you have to adjust a lot of
>> code to return error objects all the way up into the handler.  With the
>> current design, you emit error objects "sideways",
>
> But you still have to propagate an error return somewhere in order to
> short circuit the execution of the handler.
>
> So you end up doing:
>
> func1:
>   qerror_new();
>   return -1;
>
> func2:
>   err = func1();
>   if (err == -1)
>     return -1;
>
> func3:
>   err = func2();
>   if (err == -1)
>     return -1;
>
> Would it be just as easy to say:
>
> func1:
>   return qerror_new();
>
> func2:
>   obj = func1();
>   if qobject_is_qerror(obj):
>      return obj;
>
> func3:
>   obj = func2();
>   if qobject_is_qerror(obj):
>     return obj;
>
> Ultimately, this you the ability to decide in func3 whether you want
> to continue propagating the error or whether you want to take
> corrective action.

Yes, that's a sensible argument.  It's also quite impractical at this
time.  In fact, I had the same idea, and dropped it like a hot potato
when I realized how much code we'd have to touch for it.

Because besides the single call chain func3(), func2(), func1(), there
are many more, and large parts of them never heard of the monitor or
qerror/qobject.  There are places that happily ignore errors (and
nevertheless work tolerably well), and they all become memory leaks if
we change the error value from int to qobject.

Look, we're sitting in a nice hole with QMP right now.  We set ambitious
goals, we've been working on it for months, and we got quite some code,
but the result is still not useful.  It really needs to become useful as
soon as possible.

Once we made it out of this hole, I'll be game for refactoring stuff to
make it more flexible and less ugly.  But as long as I'm sitting in this
hole, I'd prefer not to dig deeper.

> The current qerror stuff is really only useful within a single
> function.  Once you start building infrastructure around qerror, it
> becomes very difficult to deal with.

[...]

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Anthony Liguori]

On 02/11/2010 10:57 AM, Markus Armbruster wrote:
> Yes, that's a sensible argument.  It's also quite impractical at this
> time.  In fact, I had the same idea, and dropped it like a hot potato
> when I realized how much code we'd have to touch for it.
>    

Can you give me an example of where it gets particularly ugly?  It 
doesn't look so bad to me.

> Because besides the single call chain func3(), func2(), func1(), there
> are many more, and large parts of them never heard of the monitor or
> qerror/qobject.  There are places that happily ignore errors (and
> nevertheless work tolerably well), and they all become memory leaks if
> we change the error value from int to qobject.
>
> Look, we're sitting in a nice hole with QMP right now.  We set ambitious
> goals, we've been working on it for months, and we got quite some code,
> but the result is still not useful.  It really needs to become useful as
> soon as possible.
>
> Once we made it out of this hole, I'll be game for refactoring stuff to
> make it more flexible and less ugly.  But as long as I'm sitting in this
> hole, I'd prefer not to dig deeper.
>    

Yeah, I can accept this argument provided that we're all in agreement 
for where we're going long term.  I'd still like to understand the 
pragmatic issue that we're facing right now though.

Regards,

Anthony Liguori

>> The current qerror stuff is really only useful within a single
>> function.  Once you start building infrastructure around qerror, it
>> becomes very difficult to deal with.
>>      
> [...]
>    

Re: [Qemu-devel] [PATCH v0 00/21]: Monitor: improve handlers error handling

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 02/11/2010 10:57 AM, Markus Armbruster wrote:
>> Yes, that's a sensible argument.  It's also quite impractical at this
>> time.  In fact, I had the same idea, and dropped it like a hot potato
>> when I realized how much code we'd have to touch for it.
>>    
>
> Can you give me an example of where it gets particularly ugly?  It
> doesn't look so bad to me.

Here's one (very) partial call graph:

* qemu_socket() uses system call convention: non-negative value on
| success, -1 and errno set on error.  It doesn't print.
|
+-* unix_listen_opts() returns non-negative value on success, -1 on
| | error.  It trashes errno.  It reports errors to stderr.
. |
. +-* qemu_chr_open_socket() returns CharDriverState on success, null on
. | | error. It doesn't report errors.  It can print an informational
  | | message to stdout.
  | |
  | |<--- indirectly through backend_table[].open
  | |
  | +-* qemu_chr_open_opts() returns CharDriverState on success, null on
  |   | error.  It reports errors to stderr.
  |   |
  |   +-* qemu_chr_open() returns CharDriverState on success, null on
  |   | | error.  It doesn't report errors.
  |   | |
  |   | +-* gdbserver_start() ...
  |   | | ...
  |   | |
  |   | +-* serial_parse() ...
  |   | | ...
  |   | |
  |   | +-* parallel_parse() ...
  |   | | ...
  |   | |
  |   | +-* virtcon_parse() ...
  |   | | ...
  |   | |
  |   | +-* debugcon_parse() ...
  |   | | ...
  |   | |
  |   | +-* malta_fpga_init() ...
  |   | | ...
  |   | |
  |   | +-* mips_malta_init() ...
  |   | | ...
  |   | |
  |   | +-* omap_uart_init() ...
  |   | | ...
  |   | |
  |   | +-* omap_uart_attach() ...
  |   | | ...
  |   | |
  |   | +-* omap_sti_init() ...
  |   | | ...
  |   | |
  |   | +-* usb_serial_init() returns USBDevice on success, null on
  |   | | | error.  It reports errors via qemu_error().
  |   | | |
  |   | | |<--- indirectly through serial_info.usbdevice_init
  |   | | |
  |   | | +-* usbdevice_create() returns USBDevice on success, null on
  |   | |   | error.  It reports errors via qemu_error().
  |   | |   |
  |   | |   +-* usb_device_add() returns 0 on success, -1 on error.  It
  |   | |     | doesn't report errors.
  |   | |     |
  |   | |     +-* usb_parse() returns 0 on success, -1 on error.  Ir
  |   | |     | | reports errors to stderr.
  |   | |     | |
  |   | |     | +-* main() calls it for -usbdevice (legacy), and wants
  |   | |     |     it to report errors to stderr. 
  |   | |     |
  |   | |     +-* do_usb_add() wants it to print errors to the monitor.
  |   | |         This handler will never be converted, because
  |   | |         device_add is more general.
  |   | |
  |   | +-* usb_braille_init() ...
  |   | | ...
  |   | |
  |   | +-* slirp_guestfwd() returns 0 on success, -1 on error.  It
  |   |   | reports errors via qemu_error().
  |   |   |
  |   |   +-* net_slirp_init() returns 0 on success, -1 on error.  It
  |   |   | | doesn't report errors.
  |   |   | |
  |   |   | +-* net_init_slirp()
  |   |   |   |
  |   |   |   |<-- indirectly through net_client_types[].init()
  |   |   |   |
  |   |   |   +-* net_client_init() returns 0 on success, -1 on error.
  |   |   |     | It reports errors via qemu_error().
  |   |   |     |
  |   |   |     +-* net_host_device_add() ...
  |   |   |     | ...
  |   |   |     |
  |   |   |     +-* net_init_client() ...
  |   |   |     | ...
  |   |   |     |
  |   |   |     +-* net_init_netdev() ...
  |   |   |     | ...
  |   |   |     |
  |   |   |     +-* qemu_pci_hot_add_nic() returns PCIDevice on success,
  |   |   |     | | null on error.  It prints errors to the monitor.
  |   |   |     | |
  |   |   |     | +-* pci_device_hot_add() wants it to use QError.
  |   |   |     |
  |   |   |     +-* usb_net_init() ...
  |   |   |       ...
  |   |   |
  |   |   +-* net_slirp_parse_legacy()
  |   |     |
  |   |     +-* net_client_parse()
  |   |       |
  |   |       +- main() calls it for -netdev and -net, and wants it to
  |   |          report errors to stderr.
  |   |
  |   +-* chardev_init_func() returns 0 on success, -1 on error.  It
  |     | doesn't report errors.
  |     |
  |     +-* main() calls it for -chardev, and wants it to report errors
  |         to stderr.
  |
  +-* unix_listen() returns non-negative value on success, -1 on
    | error.  It doesn't report errors.
    |
    +-* vnc_display_open() returns 0 on success, -1 on error.  It
      | reports errors to stderr.
      |
      +-* do_change_vnc() returns 0 on success, -1 on error.  It uses
      | | QError.
      | |
      | +-* do_change() wants it to use QError.
      |
      +-* main() calls it, and wants it to report errors to stderr.

qemu_socket() is just a system call wrapper, so let's ignore that.  The
error gets diagnosed in unix_listen_opts().  Its caller already doesn't
know (and doesn't care about) the exact error.

There are paths from unix_listen_opts() to main(), an unconverted
monitor handler that will never be converted, converted monitor
handlers, and whatever else is hiding in my dot-dot-dots.  main() wants
errors printed to stderr, unconverted handlers want them printed to the
monitor, and converted handlers want a QError object.

This is e-mail, so you get to do this for yourself: color the parts of
the graph that run on behalf of main()'s argument processing only, human
monitor only, QMP or human monitor only, all of them.  You'll see that
substantial parts have nothing to do with the monitor, and thus precious
little use for QError.

To create a QError more specific than "didn't work", you either have to
create it in unix_listen_opts(), or change it to return some other
richer error information to its callers.  Either way, you got churn
along the whole path up to the converted monitor handler.

But you have several monitor handlers, hence several paths.  You'll end
up with a big patch hairball, and get to figure out a smart way to split
it.

Sorry, I just can't do that now.


PS: The above is one of the reasons why I argued for much simpler error
reporting.  It would have let us get away with minimal changes to
qemu_error() calls, and printing to monitor or stderr would have been
trivial to convert to qemu_error().  I understand why you wanted QError,
but it makes it so much harder to get QMP into a useful state, and
before it reaches that state, it's worse than nothing: it's a drain on
precious resources, and a source of new bugs.  That's why I'm more
convinced than ever that doing QError *now* was a mistake.  But I lost
that debate, and I don't wish to reopen it.