From 65207c59d99f2260c5f1d3b9c491146616a522aa Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 5 Mar 2015 14:35:26 +0100 Subject: monitor: Drop broken, unused asynchronous command interface The asynchronous monitor command interface goes back to commit 940cc30 (Jan 2010). Added a third case to command execution. The hope back then according to the commit message was that all commands get converted to the asynchronous interface, killing off the other two cases. Didn't happen. The initial asynchronous commands balloon and info balloon were converted back to synchronous long ago (commit 96637bc and d72f32), with commit messages calling the asynchronous interface "not fully working" and "deprecated". The only other user went away in commit 3b5704b. New code generally uses synchronous commands and asynchronous events. What exactly is still "not fully working" with asynchronous commands? Well, here's a bug that defeats actual asynchronous use pretty reliably: the reply's ID is wrong (and has always been wrong) unless you use the command synchronously! To reproduce, we need an asynchronous command, so we have to go back before commit 3b5704b. Run QEMU with spice: $ qemu-system-x86_64 -nodefaults -S -spice port=5900,disable-ticketing -qmp stdio {"QMP": {"version": {"qemu": {"micro": 94, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}} Connect a spice client in another terminal: $ remote-viewer spice://localhost:5900 Set up a migration destination dummy in a third terminal: $ socat TCP-LISTEN:12345 STDIO Now paste the following into the QMP monitor: { "execute": "qmp_capabilities", "id": "i0" } { "execute": "client_migrate_info", "id": "i1", "arguments": { "protocol": "spice", "hostname": "localhost", "port": 12345 } } { "execute": "query-kvm", "id": "i2" } Produces two replies immediately, one to qmp_capabilities, and one to query-kvm: {"return": {}, "id": "i0"} {"return": {"enabled": false, "present": true}, "id": "i2"} Both are correct. Two lines of debug output from libspice-server not shown. Now EOF socat's standard input to make it close the connection. This makes the asynchronous client_migrate_info complete. It replies: {"return": {}} Bug: "id": "i1" is missing. Two lines of debug output from libspice-server not shown. Cherry on top: storage for the missing ID is leaked. Get rid of this stuff before somebody hurts himself with it. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Luiz Capitulino --- include/monitor/monitor.h | 5 ----- 1 file changed, 5 deletions(-) (limited to 'include') diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index df67d56ec0..d409b6ac56 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -16,9 +16,6 @@ extern Monitor *default_mon; #define MONITOR_USE_CONTROL 0x04 #define MONITOR_USE_PRETTY 0x08 -/* flags for monitor commands */ -#define MONITOR_CMD_ASYNC 0x0001 - int monitor_cur_is_qmp(void); void monitor_init(CharDriverState *chr, int flags); @@ -43,8 +40,6 @@ void monitor_flush(Monitor *mon); int monitor_set_cpu(int cpu_index); int monitor_get_cpu_index(void); -typedef void (MonitorCompletion)(void *opaque, QObject *ret_data); - void monitor_set_error(Monitor *mon, QError *qerror); void monitor_read_command(Monitor *mon, int show_prompt); int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func, -- cgit v1.2.3-55-g7522 From 072ebe6b0351060b33287454fdef625fe79c858f Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 5 Mar 2015 17:00:56 +0100 Subject: monitor: Use traditional command interface for HMP drive_del All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates handle_user_command() for no gain, so I'm converting these to the traditional interface. For drive_del, that's easy: hmp_drive_del() sheds its unused last parameter, and its return value, which the caller ignored anyway. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Luiz Capitulino --- blockdev.c | 9 ++++----- hmp-commands.hx | 3 +-- include/sysemu/blockdev.h | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) (limited to 'include') diff --git a/blockdev.c b/blockdev.c index 5eaf77e599..de94a8bcb3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2113,7 +2113,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, aio_context_release(aio_context); } -int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) +void hmp_drive_del(Monitor *mon, const QDict *qdict) { const char *id = qdict_get_str(qdict, "id"); BlockBackend *blk; @@ -2124,14 +2124,14 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) blk = blk_by_name(id); if (!blk) { error_report("Device '%s' not found", id); - return -1; + return; } bs = blk_bs(blk); if (!blk_legacy_dinfo(blk)) { error_report("Deleting device added with blockdev-add" " is not supported"); - return -1; + return; } aio_context = bdrv_get_aio_context(bs); @@ -2140,7 +2140,7 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) { error_report_err(local_err); aio_context_release(aio_context); - return -1; + return; } /* quiesce block driver; prevent further io */ @@ -2163,7 +2163,6 @@ int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data) } aio_context_release(aio_context); - return 0; } void qmp_block_resize(bool has_device, const char *device, diff --git a/hmp-commands.hx b/hmp-commands.hx index af2de61066..52579698af 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -178,8 +178,7 @@ ETEXI .args_type = "id:B", .params = "device", .help = "remove host block device", - .user_print = monitor_user_noop, - .mhandler.cmd_new = hmp_drive_del, + .mhandler.cmd = hmp_drive_del, }, STEXI diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h index 7ca59b5070..310415025c 100644 --- a/include/sysemu/blockdev.h +++ b/include/sysemu/blockdev.h @@ -66,5 +66,5 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type); void qmp_change_blockdev(const char *device, const char *filename, const char *format, Error **errp); void hmp_commit(Monitor *mon, const QDict *qdict); -int hmp_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data); +void hmp_drive_del(Monitor *mon, const QDict *qdict); #endif -- cgit v1.2.3-55-g7522 From 04e00c92ef75629a241ebc50537f75de0867928d Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Thu, 5 Mar 2015 17:48:49 +0100 Subject: monitor: Use trad. command interface for HMP pcie_aer_inject_error All QMP commands use the "new" handler interface (mhandler.cmd_new). Most HMP commands still use the traditional interface (mhandler.cmd), but a few use the "new" one. Complicates handle_user_command() for no gain, so I'm converting these to the traditional interface. pcie_aer_inject_error's implementation is split into the hmp_pcie_aer_inject_error() and pcie_aer_inject_error_print(). The former is a peculiar crossbreed between HMP and QMP handler. On success, it works like a QMP handler: store QDict through ret_data parameter, return 0. Printing the QDict is left to pcie_aer_inject_error_print(). On failure, it works more like an HMP handler: print error to monitor, return negative number. To convert to the traditional interface, turn pcie_aer_inject_error_print() into a command handler wrapping around hmp_pcie_aer_inject_error(). By convention, this command handler should be called hmp_pcie_aer_inject_error(), so rename the existing hmp_pcie_aer_inject_error() to do_pcie_aer_inject_error(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Luiz Capitulino --- hmp-commands.hx | 3 +-- hw/pci/pci-stub.c | 14 +------------- hw/pci/pcie_aer.c | 39 ++++++++++++++++++++++----------------- include/sysemu/sysemu.h | 4 +--- 4 files changed, 25 insertions(+), 35 deletions(-) (limited to 'include') diff --git a/hmp-commands.hx b/hmp-commands.hx index eff2f976d3..3d7dfccf7c 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1183,8 +1183,7 @@ ETEXI " = error string or 32bit\n\t\t\t" " = 32bit x 4\n\t\t\t" " = 32bit x 4", - .user_print = pcie_aer_inject_error_print, - .mhandler.cmd_new = hmp_pcie_aer_inject_error, + .mhandler.cmd = hmp_pcie_aer_inject_error, }, STEXI diff --git a/hw/pci/pci-stub.c b/hw/pci/pci-stub.c index 5e564c3a87..f8f237e823 100644 --- a/hw/pci/pci-stub.c +++ b/hw/pci/pci-stub.c @@ -29,19 +29,7 @@ PciInfoList *qmp_query_pci(Error **errp) return NULL; } -static void pci_error_message(Monitor *mon) +void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) { monitor_printf(mon, "PCI devices not supported\n"); } - -int hmp_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data) -{ - pci_error_message(mon); - return -ENOSYS; -} - -void pcie_aer_inject_error_print(Monitor *mon, const QObject *data) -{ - pci_error_message(mon); -} diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c index b48c09cd11..c8dea8ed9c 100644 --- a/hw/pci/pcie_aer.c +++ b/hw/pci/pcie_aer.c @@ -815,21 +815,6 @@ const VMStateDescription vmstate_pcie_aer_log = { } }; -void pcie_aer_inject_error_print(Monitor *mon, const QObject *data) -{ - QDict *qdict; - int devfn; - assert(qobject_type(data) == QTYPE_QDICT); - qdict = qobject_to_qdict(data); - - devfn = (int)qdict_get_int(qdict, "devfn"); - monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n", - qdict_get_str(qdict, "id"), - qdict_get_str(qdict, "root_bus"), - (int) qdict_get_int(qdict, "bus"), - PCI_SLOT(devfn), PCI_FUNC(devfn)); -} - typedef struct PCIEAERErrorName { const char *name; uint32_t val; @@ -962,8 +947,8 @@ static int pcie_aer_parse_error_string(const char *error_name, return -EINVAL; } -int hmp_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data) +static int do_pcie_aer_inject_error(Monitor *mon, + const QDict *qdict, QObject **ret_data) { const char *id = qdict_get_str(qdict, "id"); const char *error_name; @@ -1035,3 +1020,23 @@ int hmp_pcie_aer_inject_error(Monitor *mon, return 0; } + +void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict) +{ + QObject *data; + int devfn; + + if (do_pcie_aer_inject_error(mon, qdict, &data) < 0) { + return; + } + + assert(qobject_type(data) == QTYPE_QDICT); + qdict = qobject_to_qdict(data); + + devfn = (int)qdict_get_int(qdict, "devfn"); + monitor_printf(mon, "OK id: %s root bus: %s, bus: %x devfn: %x.%x\n", + qdict_get_str(qdict, "id"), + qdict_get_str(qdict, "root_bus"), + (int) qdict_get_int(qdict, "bus"), + PCI_SLOT(devfn), PCI_FUNC(devfn)); +} diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 8a52934728..e10c2c5217 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -161,9 +161,7 @@ extern unsigned int nb_prom_envs; void hmp_drive_add(Monitor *mon, const QDict *qdict); /* pcie aer error injection */ -void pcie_aer_inject_error_print(Monitor *mon, const QObject *data); -int hmp_pcie_aer_inject_error(Monitor *mon, - const QDict *qdict, QObject **ret_data); +void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict); /* serial ports */ -- cgit v1.2.3-55-g7522 From 489653b5db17679fd61b740dd289c798bb25d7b9 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 6 Mar 2015 20:01:05 +0100 Subject: monitor: Change return type of monitor_cur_is_qmp() to bool Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Luiz Capitulino --- include/monitor/monitor.h | 2 +- monitor.c | 6 ++++-- stubs/mon-is-qmp.c | 4 ++-- 3 files changed, 7 insertions(+), 5 deletions(-) (limited to 'include') diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index d409b6ac56..57f8394a94 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -16,7 +16,7 @@ extern Monitor *default_mon; #define MONITOR_USE_CONTROL 0x04 #define MONITOR_USE_PRETTY 0x08 -int monitor_cur_is_qmp(void); +bool monitor_cur_is_qmp(void); void monitor_init(CharDriverState *chr, int flags); diff --git a/monitor.c b/monitor.c index be18ac1e44..c7baa9198c 100644 --- a/monitor.c +++ b/monitor.c @@ -239,8 +239,10 @@ static inline bool monitor_is_qmp(const Monitor *mon) return (mon->flags & MONITOR_USE_CONTROL); } -/* Return non-zero iff we have a current monitor, and it is in QMP mode. */ -int monitor_cur_is_qmp(void) +/** + * Is the current monitor, if any, a QMP monitor? + */ +bool monitor_cur_is_qmp(void) { return cur_mon && monitor_is_qmp(cur_mon); } diff --git a/stubs/mon-is-qmp.c b/stubs/mon-is-qmp.c index 1f0a8fd98a..1ef136ab1d 100644 --- a/stubs/mon-is-qmp.c +++ b/stubs/mon-is-qmp.c @@ -1,7 +1,7 @@ #include "qemu-common.h" #include "monitor/monitor.h" -int monitor_cur_is_qmp(void) +bool monitor_cur_is_qmp(void) { - return 0; + return false; } -- cgit v1.2.3-55-g7522