[Qemu-devel] [PULL 20/24] qmp: Wean off qerror_report()

[Markus Armbruster]

The traditional QMP command handler interface

    int qmp_FOO(Monitor *mon, const QDict *params, QObject **ret_data);

doesn't provide for returning an Error object.  Instead, the handler
is expected to stash it in the monitor with qerror_report().

When we rebased QMP on top of QAPI, we didn't change this interface.
Instead, commit 776574d introduced "middle mode" as a temporary aid
for converting existing QMP commands to QAPI one by one.  More than
three years later, we're still using it.

Middle mode has two effects:

* Instead of the native input marshallers

      static void qmp_marshal_input_FOO(QDict *, QObject **, Error **)

  it generates input marshallers conforming to the traditional QMP
  command handler interface.

* It suppresses generation of code to register them with
  qmp_register_command()

  This permits giving them internal linkage.

As long as we need qmp-commands.hx, we can't use the registry behind
qmp_register_command(), so the latter has to stay for now.

The former has to go to get rid of qerror_report().  Changing all QMP
commands to fit the QAPI mold in one go was impractical back when we
started, but by now there are just a few stragglers left:
do_qmp_capabilities(), qmp_qom_set(), qmp_qom_get(), qmp_object_add(),
qmp_netdev_add(), do_device_add().

Switch middle mode to generate native input marshallers, and adapt the
stragglers.  Simplifies both the monitor code and the stragglers.

Rename do_qmp_capabilities() to qmp_capabilities(), and
do_device_add() to qmp_device_add, because that's how QMP command
handlers are named today.

Signed-off-by: Markus Armbruster <address@hidden>
Reviewed-by: Stefan Hajnoczi <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Reviewed-by: Luiz Capitulino <address@hidden>
---
 hmp.c                     |  5 ++++-
 include/monitor/monitor.h |  7 +++---
 include/monitor/qdev.h    |  3 ++-
 include/net/net.h         |  2 +-
 monitor.c                 | 24 ++++++---------------
 net/net.c                 | 16 ++++++--------
 qdev-monitor.c            | 15 ++++++-------
 qmp-commands.hx           |  4 ++--
 qmp.c                     | 55 +++++++++++------------------------------------
 scripts/qapi-commands.py  | 41 ++++++-----------------------------
 util/qemu-error.c         |  4 ++--
 11 files changed, 50 insertions(+), 126 deletions(-)

diff --git a/hmp.c b/hmp.c
index 901a893..fd10ab5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1507,7 +1507,10 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
 
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
-    do_device_add(mon, qdict, NULL);
+    Error *err = NULL;
+
+    qmp_device_add((QDict *)qdict, NULL, &err);
+    hmp_handle_error(mon, &err);
 }
 
 void hmp_device_del(Monitor *mon, const QDict *qdict)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 88644ce..33c28a4 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -44,10 +44,9 @@ void monitor_read_command(Monitor *mon, int show_prompt);
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque);
 
-int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
-
-int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
-int qmp_object_add(Monitor *mon, const QDict *qdict, QObject **ret);
+void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp);
+void qmp_qom_get(QDict *qdict, QObject **ret, Error **errp);
+void qmp_object_add(QDict *qdict, QObject **ret, Error **errp);
 void object_add(const char *type, const char *id, const QDict *qdict,
                 Visitor *v, Error **errp);
 
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 2ce8578..dc796c8 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -9,7 +9,8 @@
 void hmp_info_qtree(Monitor *mon, const QDict *qdict);
 void hmp_info_qdm(Monitor *mon, const QDict *qdict);
 void hmp_info_qom_tree(Monitor *mon, const QDict *dict);
-int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
+void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp);
+
 int qdev_device_help(QemuOpts *opts);
 DeviceState *qdev_device_add(QemuOpts *opts, Error **errp);
 
diff --git a/include/net/net.h b/include/net/net.h
index 4306252..6a6cbef 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -197,7 +197,7 @@ void net_cleanup(void);
 void hmp_host_net_add(Monitor *mon, const QDict *qdict);
 void hmp_host_net_remove(Monitor *mon, const QDict *qdict);
 void netdev_add(QemuOpts *opts, Error **errp);
-int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret);
+void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp);
 
 int net_hub_id_for_client(NetClientState *nc, int *id);
 NetClientState *net_hub_port_find(int hub_id);
diff --git a/monitor.c b/monitor.c
index 4cdc873..6a0ed80 100644
--- a/monitor.c
+++ b/monitor.c
@@ -122,7 +122,7 @@ typedef struct mon_cmd_t {
     const char *help;
     union {
         void (*cmd)(Monitor *mon, const QDict *qdict);
-        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
+        void (*cmd_new)(QDict *params, QObject **ret_data, Error **errp);
     } mhandler;
     /* @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
@@ -563,11 +563,9 @@ static void monitor_qapi_event_init(void)
     qmp_event_set_func_emit(monitor_qapi_event_queue);
 }
 
-static int do_qmp_capabilities(Monitor *mon, const QDict *params,
-                               QObject **ret_data)
+static void qmp_capabilities(QDict *params, QObject **ret_data, Error **errp)
 {
-    mon->qmp.in_command_mode = true;
-    return 0;
+    cur_mon->qmp.in_command_mode = true;
 }
 
 static void handle_hmp_command(Monitor *mon, const char *cmdline);
@@ -4725,7 +4723,7 @@ static int monitor_can_read(void *opaque)
 static bool invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd,
                              Error **errp)
 {
-    bool is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities;
+    bool is_cap = cmd->mhandler.cmd_new == qmp_capabilities;
 
     if (is_cap && mon->qmp.in_command_mode) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
@@ -5044,17 +5042,7 @@ static void handle_qmp_command(JSONMessageParser 
*parser, QList *tokens)
         goto err_out;
     }
 
-    if (cmd->mhandler.cmd_new(mon, args, &data)) {
-        /* Command failed... */
-        if (!mon->error) {
-            /* ... without setting an error, so make one up */
-            error_setg(&local_err, QERR_UNDEFINED_ERROR);
-        }
-    }
-    if (mon->error) {
-        error_set(&local_err, mon->error->err_class, "%s",
-                  mon->error->err_msg);
-    }
+    cmd->mhandler.cmd_new(args, &data, &local_err);
 
 err_out:
     monitor_protocol_emitter(mon, data, local_err);
@@ -5126,7 +5114,7 @@ static QObject *get_qmp_greeting(void)
 {
     QObject *ver = NULL;
 
-    qmp_marshal_input_query_version(NULL, NULL, &ver);
+    qmp_marshal_input_query_version(NULL, &ver, NULL);
     return qobject_from_jsonf("{'QMP':{'version': %p,'capabilities': 
[]}}",ver);
 }
 
diff --git a/net/net.c b/net/net.c
index 7793366..a5e2da2 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1090,7 +1090,7 @@ void netdev_add(QemuOpts *opts, Error **errp)
     net_client_init(opts, 1, errp);
 }
 
-int qmp_netdev_add(Monitor *mon, const QDict *qdict, QObject **ret)
+void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp)
 {
     Error *local_err = NULL;
     QemuOptsList *opts_list;
@@ -1098,26 +1098,22 @@ int qmp_netdev_add(Monitor *mon, const QDict *qdict, 
QObject **ret)
 
     opts_list = qemu_find_opts_err("netdev", &local_err);
     if (local_err) {
-        goto exit_err;
+        goto out;
     }
 
     opts = qemu_opts_from_qdict(opts_list, qdict, &local_err);
     if (local_err) {
-        goto exit_err;
+        goto out;
     }
 
     netdev_add(opts, &local_err);
     if (local_err) {
         qemu_opts_del(opts);
-        goto exit_err;
+        goto out;
     }
 
-    return 0;
-
-exit_err:
-    qerror_report_err(local_err);
-    error_free(local_err);
-    return -1;
+out:
+    error_propagate(errp, local_err);
 }
 
 void qmp_netdev_del(const char *id, Error **errp)
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e3bf84a..124956e 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -756,7 +756,7 @@ void hmp_info_qom_tree(Monitor *mon, const QDict *dict)
     print_qom_composition(mon, obj, 0);
 }
 
-int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
 {
     Error *local_err = NULL;
     QemuOpts *opts;
@@ -764,23 +764,20 @@ int do_device_add(Monitor *mon, const QDict *qdict, 
QObject **ret_data)
 
     opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
+        error_propagate(errp, local_err);
+        return;
     }
     if (!monitor_cur_is_qmp() && qdev_device_help(opts)) {
         qemu_opts_del(opts);
-        return 0;
+        return;
     }
     dev = qdev_device_add(opts, &local_err);
     if (!dev) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         qemu_opts_del(opts);
-        return -1;
+        return;
     }
     object_unref(OBJECT(dev));
-    return 0;
 }
 
 void qmp_device_del(const char *id, Error **errp)
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1db6524..3ffa612 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -276,7 +276,7 @@ EQMP
         .args_type  = "device:O",
         .params     = "driver[,prop=value][,...]",
         .help       = "add device, like -device on the command line",
-        .mhandler.cmd_new = do_device_add,
+        .mhandler.cmd_new = qmp_device_add,
     },
 
 SQMP
@@ -1983,7 +1983,7 @@ EQMP
         .args_type  = "",
         .params     = "",
         .help       = "enable QMP capabilities",
-        .mhandler.cmd_new = do_qmp_capabilities,
+        .mhandler.cmd_new = qmp_capabilities,
     },
 
 SQMP
diff --git a/qmp.c b/qmp.c
index 3ffb349..a046b68 100644
--- a/qmp.c
+++ b/qmp.c
@@ -227,57 +227,37 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, 
Error **errp)
 }
 
 /* FIXME: teach qapi about how to pass through Visitors */
-int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret)
+void qmp_qom_set(QDict *qdict, QObject **ret, Error **errp)
 {
     const char *path = qdict_get_str(qdict, "path");
     const char *property = qdict_get_str(qdict, "property");
     QObject *value = qdict_get(qdict, "value");
-    Error *local_err = NULL;
     Object *obj;
 
     obj = object_resolve_path(path, NULL);
     if (!obj) {
-        error_set(&local_err, ERROR_CLASS_DEVICE_NOT_FOUND,
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", path);
-        goto out;
+        return;
     }
 
-    object_property_set_qobject(obj, value, property, &local_err);
-
-out:
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
-    return 0;
+    object_property_set_qobject(obj, value, property, errp);
 }
 
-int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret)
+void qmp_qom_get(QDict *qdict, QObject **ret, Error **errp)
 {
     const char *path = qdict_get_str(qdict, "path");
     const char *property = qdict_get_str(qdict, "property");
-    Error *local_err = NULL;
     Object *obj;
 
     obj = object_resolve_path(path, NULL);
     if (!obj) {
-        error_set(&local_err, ERROR_CLASS_DEVICE_NOT_FOUND,
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", path);
-        goto out;
+        return;
     }
 
-    *ret = object_property_get_qobject(obj, property, &local_err);
-
-out:
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
-    return 0;
+    *ret = object_property_get_qobject(obj, property, errp);
 }
 
 void qmp_set_password(const char *protocol, const char *password,
@@ -673,36 +653,25 @@ out:
     object_unref(obj);
 }
 
-int qmp_object_add(Monitor *mon, const QDict *qdict, QObject **ret)
+void qmp_object_add(QDict *qdict, QObject **ret, Error **errp)
 {
     const char *type = qdict_get_str(qdict, "qom-type");
     const char *id = qdict_get_str(qdict, "id");
     QObject *props = qdict_get(qdict, "props");
     const QDict *pdict = NULL;
-    Error *local_err = NULL;
     QmpInputVisitor *qiv;
 
     if (props) {
         pdict = qobject_to_qdict(props);
         if (!pdict) {
-            error_setg(&local_err, QERR_INVALID_PARAMETER_TYPE,
-                       "props", "dict");
-            goto out;
+            error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
+            return;
         }
     }
 
     qiv = qmp_input_visitor_new(props);
-    object_add(type, id, pdict, qmp_input_get_visitor(qiv), &local_err);
+    object_add(type, id, pdict, qmp_input_get_visitor(qiv), errp);
     qmp_input_visitor_cleanup(qiv);
-
-out:
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
-    return 0;
 }
 
 void qmp_object_del(const char *id, Error **errp)
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 1c1d3aa..a451a51 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -195,12 +195,10 @@ out:
     return ret
 
 def gen_marshal_input_decl(name, args, ret_type, middle_mode):
-    if middle_mode:
-        return 'int qmp_marshal_input_%s(Monitor *mon, const QDict *qdict, 
QObject **ret)' % c_name(name)
-    else:
-        return 'static void qmp_marshal_input_%s(QDict *args, QObject **ret, 
Error **errp)' % c_name(name)
-
-
+    ret = 'void qmp_marshal_input_%s(QDict *args, QObject **ret, Error 
**errp)' % c_name(name)
+    if not middle_mode:
+        ret = "static " + ret
+    return ret
 
 def gen_marshal_input(name, args, ret_type, middle_mode):
     hdr = gen_marshal_input_decl(name, args, ret_type, middle_mode)
@@ -212,11 +210,6 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
 ''',
                 header=hdr)
 
-    if middle_mode:
-        ret += mcgen('''
-    QDict *args = (QDict *)qdict;
-''')
-
     if ret_type:
         if is_c_ptr(ret_type):
             retval = "    %s retval = NULL;" % c_type(ret_type)
@@ -253,35 +246,13 @@ def gen_marshal_input(name, args, ret_type, middle_mode):
 
 out:
 ''')
-    if not middle_mode:
-        ret += mcgen('''
-    error_propagate(errp, local_err);
-''')
     ret += mcgen('''
+    error_propagate(errp, local_err);
 %(visitor_input_block_cleanup)s
+}
 ''',
                  visitor_input_block_cleanup=gen_visitor_input_block(args,
                                                                      
dealloc=True))
-
-    if middle_mode:
-        ret += mcgen('''
-
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-    return 0;
-''')
-    else:
-        ret += mcgen('''
-    return;
-''')
-
-    ret += mcgen('''
-}
-''')
-
     return ret
 
 def gen_registry(commands):
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 9bba5f5..16d2d07 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -202,7 +202,7 @@ bool enable_timestamp_msg;
  * Format arguments like vsprintf().  The result should not contain
  * newlines.
  * Prepend the current location and append a newline.
- * It's wrong to call this in a QMP monitor.  Use qerror_report() there.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
  */
 void error_vreport(const char *fmt, va_list ap)
 {
@@ -226,7 +226,7 @@ void error_vreport(const char *fmt, va_list ap)
  * Format arguments like sprintf().  The result should not contain
  * newlines.
  * Prepend the current location and append a newline.
- * It's wrong to call this in a QMP monitor.  Use qerror_report() there.
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
  */
 void error_report(const char *fmt, ...)
 {
-- 
1.9.3