From b25d81ba332dc06757d40ffe64944ba27082de0a Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 17:07:15 +0100 Subject: [PATCH 1/9] qmp hmp: Factor out common "using spice" test Into qemu_using_spice(). For want of a better place, put it next the existing monitor command handler dummies in qemu-spice.h. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Gerd Hoffmann --- include/ui/qemu-spice.h | 10 ++++++++++ monitor.c | 5 +++-- qmp.c | 11 +++-------- 3 files changed, 16 insertions(+), 10 deletions(-) diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h index a93b4b2572..db7926d50b 100644 --- a/include/ui/qemu-spice.h +++ b/include/ui/qemu-spice.h @@ -88,4 +88,14 @@ static inline int qemu_spice_display_add_client(int csock, int skipauth, #endif /* CONFIG_SPICE */ +static inline bool qemu_using_spice(Error **errp) +{ + if (!using_spice) { + /* correct one? spice isn't a device ,,, */ + error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice"); + return false; + } + return true; +} + #endif /* QEMU_SPICE_H */ diff --git a/monitor.c b/monitor.c index 7e4f605e6d..8323de3ebc 100644 --- a/monitor.c +++ b/monitor.c @@ -1095,11 +1095,12 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, const char *subject = qdict_get_try_str(qdict, "cert-subject"); int port = qdict_get_try_int(qdict, "port", -1); int tls_port = qdict_get_try_int(qdict, "tls-port", -1); + Error *err; int ret; if (strcmp(protocol, "spice") == 0) { - if (!using_spice) { - qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice"); + if (!qemu_using_spice(&err)) { + qerror_report_err(err); return -1; } diff --git a/qmp.c b/qmp.c index 963305c269..ef155ff3aa 100644 --- a/qmp.c +++ b/qmp.c @@ -287,9 +287,7 @@ void qmp_set_password(const char *protocol, const char *password, } if (strcmp(protocol, "spice") == 0) { - if (!using_spice) { - /* correct one? spice isn't a device ,,, */ - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice"); + if (!qemu_using_spice(errp)) { return; } rc = qemu_spice_set_passwd(password, fail_if_connected, @@ -335,9 +333,7 @@ void qmp_expire_password(const char *protocol, const char *whenstr, } if (strcmp(protocol, "spice") == 0) { - if (!using_spice) { - /* correct one? spice isn't a device ,,, */ - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice"); + if (!qemu_using_spice(errp)) { return; } rc = qemu_spice_set_pw_expire(when); @@ -575,8 +571,7 @@ void qmp_add_client(const char *protocol, const char *fdname, } if (strcmp(protocol, "spice") == 0) { - if (!using_spice) { - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice"); + if (!qemu_using_spice(errp)) { close(fd); return; } From cfa9bb236911eaa30ac58072105881a65cbbb612 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 17:21:45 +0100 Subject: [PATCH 2/9] qmp hmp: Improve error messages when SPICE is not in use Commit 7572150 adopted QERR_DEVICE_NOT_ACTIVE for the purpose, probably because adding another error seemed cumbersome overkill. Produces "No spice device has been activated", which is awkward. We've since abandoned our quest for "rich" error objects. Time to undo the damage to this error message. Replace it by "SPICE is not in use". Keep the stupid DeviceNotActive ErrorClass for compatibility, even though Libvirt doesn't use it. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Gerd Hoffmann --- include/ui/qemu-spice.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/ui/qemu-spice.h b/include/ui/qemu-spice.h index db7926d50b..762e063125 100644 --- a/include/ui/qemu-spice.h +++ b/include/ui/qemu-spice.h @@ -91,8 +91,8 @@ static inline int qemu_spice_display_add_client(int csock, int skipauth, static inline bool qemu_using_spice(Error **errp) { if (!using_spice) { - /* correct one? spice isn't a device ,,, */ - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice"); + error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, + "SPICE is not in use"); return false; } return true; From 206addd58f251666f5b64d43957ddcba465c0ef1 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 15:46:39 +0100 Subject: [PATCH 3/9] hmp: Compile hmp_info_spice() only with CONFIG_SPICE It's dead code when CONFIG_SPICE is off. If it wasn't, it would crash dereferencing the null pointer returned by the qmp_query_spice() dummy in qmp.c. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Gerd Hoffmann --- hmp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hmp.c b/hmp.c index 481be8019b..a42c5c07be 100644 --- a/hmp.c +++ b/hmp.c @@ -535,6 +535,7 @@ out: qapi_free_VncInfo(info); } +#ifdef CONFIG_SPICE void hmp_info_spice(Monitor *mon, const QDict *qdict) { SpiceChannelList *chan; @@ -581,6 +582,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict) out: qapi_free_SpiceInfo(info); } +#endif void hmp_info_balloon(Monitor *mon, const QDict *qdict) { From ad0ec14bad645d9c0402047e858ea323151c8e9b Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 15:56:11 +0100 Subject: [PATCH 4/9] qmp: Clean up qmp_query_spice() #ifndef !CONFIG_SPICE dummy QMP command query-spice exists only #ifdef CONFIG_SPICE. Due to QAPI limitations, we need a dummy function anyway, but it's unreachable. Our current dummy function goes out of its way to produce the exact same error as the QMP core does for unknown commands. Cute, but both unclean and unnecessary. Replace by straight abort(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Gerd Hoffmann --- qmp.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/qmp.c b/qmp.c index ef155ff3aa..7f2d25a492 100644 --- a/qmp.c +++ b/qmp.c @@ -137,14 +137,18 @@ VncInfo *qmp_query_vnc(Error **errp) #endif #ifndef CONFIG_SPICE -/* If SPICE support is enabled, the "true" query-spice command is - defined in the SPICE subsystem. Also note that we use a small - trick to maintain query-spice's original behavior, which is not - to be available in the namespace if SPICE is not compiled in */ +/* + * qmp-commands.hx ensures that QMP command query-spice exists only + * #ifdef CONFIG_SPICE. Necessary for an accurate query-commands + * result. However, the QAPI schema is blissfully unaware of that, + * and the QAPI code generator happily generates a dead + * qmp_marshal_input_query_spice() that calls qmp_query_spice(). + * Provide it one, or else linking fails. + * FIXME Educate the QAPI schema on CONFIG_SPICE. + */ SpiceInfo *qmp_query_spice(Error **errp) { - error_set(errp, QERR_COMMAND_NOT_FOUND, "query-spice"); - return NULL; + abort(); }; #endif From bb5224edfb1e6f94aabaa8070d11c8f95f8ae277 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 16:14:04 +0100 Subject: [PATCH 5/9] qmp: Simplify recognition of capability negotiation command Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- monitor.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/monitor.c b/monitor.c index 8323de3ebc..2ac40c9e2b 100644 --- a/monitor.c +++ b/monitor.c @@ -4783,9 +4783,9 @@ static int monitor_can_read(void *opaque) return (mon->suspend_cnt == 0) ? 1 : 0; } -static int invalid_qmp_mode(const Monitor *mon, const char *cmd_name) +static int invalid_qmp_mode(const Monitor *mon, const mon_cmd_t *cmd) { - int is_cap = compare_cmd(cmd_name, "qmp_capabilities"); + int is_cap = cmd->mhandler.cmd_new == do_qmp_capabilities; return (qmp_cmd_mode(mon) ? is_cap : !is_cap); } @@ -5079,13 +5079,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) cmd_name = qdict_get_str(input, "execute"); trace_handle_qmp_command(mon, cmd_name); - if (invalid_qmp_mode(mon, cmd_name)) { - qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); - goto err_out; - } - cmd = qmp_find_cmd(cmd_name); - if (!cmd) { + if (!cmd || invalid_qmp_mode(mon, cmd)) { qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); goto err_out; } From a6c90cbccdc07d51d4bbff71d2680f1a2c287370 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 16:16:35 +0100 Subject: [PATCH 6/9] qmp: Eliminate silly QERR_COMMAND_NOT_FOUND macro The QERR_ macros are leftovers from the days of "rich" error objects. They're used with error_set() and qerror_report(), and expand into the first *two* arguments. This trickiness has become pointless. Clean this one up. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- include/qapi/qmp/qerror.h | 3 --- monitor.c | 3 ++- qapi/qmp-dispatch.c | 3 ++- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 0ca6cbd0e6..28f980eb22 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -52,9 +52,6 @@ void qerror_report_err(Error *err); #define QERR_BUS_NOT_FOUND \ ERROR_CLASS_GENERIC_ERROR, "Bus '%s' not found" -#define QERR_COMMAND_NOT_FOUND \ - ERROR_CLASS_COMMAND_NOT_FOUND, "The command %s has not been found" - #define QERR_DEVICE_ENCRYPTED \ ERROR_CLASS_DEVICE_ENCRYPTED, "'%s' (%s) is encrypted" diff --git a/monitor.c b/monitor.c index 2ac40c9e2b..2e2b0e5b37 100644 --- a/monitor.c +++ b/monitor.c @@ -5081,7 +5081,8 @@ static void handle_qmp_command(JSONMessageParser *parser, QList *tokens) trace_handle_qmp_command(mon, cmd_name); cmd = qmp_find_cmd(cmd_name); if (!cmd || invalid_qmp_mode(mon, cmd)) { - qerror_report(QERR_COMMAND_NOT_FOUND, cmd_name); + qerror_report(ERROR_CLASS_COMMAND_NOT_FOUND, + "The command %s has not been found", cmd_name); goto err_out; } diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c index 168b083c87..222742013f 100644 --- a/qapi/qmp-dispatch.c +++ b/qapi/qmp-dispatch.c @@ -76,7 +76,8 @@ static QObject *do_qmp_dispatch(QObject *request, Error **errp) command = qdict_get_str(dict, "execute"); cmd = qmp_find_command(command); if (cmd == NULL) { - error_set(errp, QERR_COMMAND_NOT_FOUND, command); + error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND, + "The command %s has not been found", command); return NULL; } if (!cmd->enabled) { From 6502a14734e71b2f6dd079b0a1e546e6aa2d2f8d Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 17:43:25 +0100 Subject: [PATCH 7/9] balloon: Inline qemu_balloon(), qemu_balloon_status() ... and simplify a bit. Permits factoring out common error checks in the next commit. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- balloon.c | 36 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 26 deletions(-) diff --git a/balloon.c b/balloon.c index b70da4fa29..2884c2d37a 100644 --- a/balloon.c +++ b/balloon.c @@ -62,25 +62,6 @@ void qemu_remove_balloon_handler(void *opaque) balloon_opaque = NULL; } -static int qemu_balloon(ram_addr_t target) -{ - if (!balloon_event_fn) { - return 0; - } - trace_balloon_event(balloon_opaque, target); - balloon_event_fn(balloon_opaque, target); - return 1; -} - -static int qemu_balloon_status(BalloonInfo *info) -{ - if (!balloon_stat_fn) { - return 0; - } - balloon_stat_fn(balloon_opaque, info); - return 1; -} - BalloonInfo *qmp_query_balloon(Error **errp) { BalloonInfo *info; @@ -90,30 +71,33 @@ BalloonInfo *qmp_query_balloon(Error **errp) return NULL; } - info = g_malloc0(sizeof(*info)); - - if (qemu_balloon_status(info) == 0) { + if (!balloon_stat_fn) { error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon"); - qapi_free_BalloonInfo(info); return NULL; } + info = g_malloc0(sizeof(*info)); + balloon_stat_fn(balloon_opaque, info); return info; } -void qmp_balloon(int64_t value, Error **errp) +void qmp_balloon(int64_t target, Error **errp) { if (kvm_enabled() && !kvm_has_sync_mmu()) { error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon"); return; } - if (value <= 0) { + if (target <= 0) { error_set(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size"); return; } - if (qemu_balloon(value) == 0) { + if (!balloon_event_fn) { error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon"); + return; } + + trace_balloon_event(balloon_opaque, target); + balloon_event_fn(balloon_opaque, target); } From 422e0501a842fb4b3f42494f341707e809c3c6ad Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 17:44:14 +0100 Subject: [PATCH 8/9] balloon: Factor out common "is balloon active" test Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- balloon.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/balloon.c b/balloon.c index 2884c2d37a..728bb707bf 100644 --- a/balloon.c +++ b/balloon.c @@ -36,6 +36,19 @@ static QEMUBalloonEvent *balloon_event_fn; static QEMUBalloonStatus *balloon_stat_fn; static void *balloon_opaque; +static bool have_ballon(Error **errp) +{ + if (kvm_enabled() && !kvm_has_sync_mmu()) { + error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon"); + return false; + } + if (!balloon_event_fn) { + error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon"); + return false; + } + return true; +} + int qemu_add_balloon_handler(QEMUBalloonEvent *event_func, QEMUBalloonStatus *stat_func, void *opaque) { @@ -66,13 +79,7 @@ BalloonInfo *qmp_query_balloon(Error **errp) { BalloonInfo *info; - if (kvm_enabled() && !kvm_has_sync_mmu()) { - error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon"); - return NULL; - } - - if (!balloon_stat_fn) { - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon"); + if (!have_ballon(errp)) { return NULL; } @@ -83,8 +90,7 @@ BalloonInfo *qmp_query_balloon(Error **errp) void qmp_balloon(int64_t target, Error **errp) { - if (kvm_enabled() && !kvm_has_sync_mmu()) { - error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon"); + if (!have_ballon(errp)) { return; } @@ -92,11 +98,6 @@ void qmp_balloon(int64_t target, Error **errp) error_set(errp, QERR_INVALID_PARAMETER_VALUE, "target", "a size"); return; } - - if (!balloon_event_fn) { - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon"); - return; - } trace_balloon_event(balloon_opaque, target); balloon_event_fn(balloon_opaque, target); From 2ad28a088d2cc8fd404dfa58fa1b80f9225425ff Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Tue, 13 Jan 2015 17:50:23 +0100 Subject: [PATCH 9/9] balloon: Eliminate silly QERR_ macros The QERR_ macros are leftovers from the days of "rich" error objects. They're used with error_set() and qerror_report(), and expand into the first *two* arguments. This trickiness has become pointless. Clean up the balloon ones. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Signed-off-by: Markus Armbruster --- balloon.c | 6 ++++-- include/qapi/qmp/qerror.h | 6 ------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/balloon.c b/balloon.c index 728bb707bf..dea19a470a 100644 --- a/balloon.c +++ b/balloon.c @@ -39,11 +39,13 @@ static void *balloon_opaque; static bool have_ballon(Error **errp) { if (kvm_enabled() && !kvm_has_sync_mmu()) { - error_set(errp, QERR_KVM_MISSING_CAP, "synchronous MMU", "balloon"); + error_set(errp, ERROR_CLASS_KVM_MISSING_CAP, + "Using KVM without synchronous MMU, balloon unavailable"); return false; } if (!balloon_event_fn) { - error_set(errp, QERR_DEVICE_NOT_ACTIVE, "balloon"); + error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, + "No balloon device has been activated"); return false; } return true; diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h index 28f980eb22..eeaf0cb981 100644 --- a/include/qapi/qmp/qerror.h +++ b/include/qapi/qmp/qerror.h @@ -70,9 +70,6 @@ void qerror_report_err(Error *err); #define QERR_DEVICE_NO_HOTPLUG \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging" -#define QERR_DEVICE_NOT_ACTIVE \ - ERROR_CLASS_DEVICE_NOT_ACTIVE, "No %s device has been activated" - #define QERR_DEVICE_NOT_ENCRYPTED \ ERROR_CLASS_GENERIC_ERROR, "Device '%s' is not encrypted" @@ -109,9 +106,6 @@ void qerror_report_err(Error *err); #define QERR_JSON_PARSING \ ERROR_CLASS_GENERIC_ERROR, "Invalid JSON syntax" -#define QERR_KVM_MISSING_CAP \ - ERROR_CLASS_KVM_MISSING_CAP, "Using KVM without %s, %s unavailable" - #define QERR_MIGRATION_ACTIVE \ ERROR_CLASS_GENERIC_ERROR, "There's a migration process in progress"