From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z3mJ3-00063L-My for qemu-devel@nongnu.org; Sat, 13 Jun 2015 10:21:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z3mJ0-0004s4-Uv for qemu-devel@nongnu.org; Sat, 13 Jun 2015 10:21:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48363) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z3mJ0-0004rg-L4 for qemu-devel@nongnu.org; Sat, 13 Jun 2015 10:21:06 -0400 From: Markus Armbruster Date: Sat, 13 Jun 2015 16:20:54 +0200 Message-Id: <1434205258-1932-8-git-send-email-armbru@redhat.com> In-Reply-To: <1434205258-1932-1-git-send-email-armbru@redhat.com> References: <1434205258-1932-1-git-send-email-armbru@redhat.com> Subject: [Qemu-devel] [PATCH 07/11] qmp: Wean off qerror_report() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: kwolf@redhat.com, mdroth@linux.vnet.ibm.com, stefanha@redhat.com, lcapitulino@redhat.com 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 --- 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 2b734ea..de934bd 100644 --- a/hmp.c +++ b/hmp.c @@ -1503,7 +1503,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 57f8394..1955d11 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -45,10 +45,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 e66ca03..9084287 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -191,7 +191,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 92e48d4..6719dc3 100644 --- a/monitor.c +++ b/monitor.c @@ -125,7 +125,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 @@ -567,11 +567,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); @@ -4733,7 +4731,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, @@ -5052,17 +5050,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); @@ -5134,7 +5122,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 2089df2..a18b8c3 100644 --- a/net/net.c +++ b/net/net.c @@ -1072,7 +1072,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; @@ -1080,26 +1080,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 2b3fd54..4447307 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 c97d0d7..0576939 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 @@ -1982,7 +1982,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 c13a5d2..4acc222 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