monitor: enable OOB by default
v6 cover monitor: enable OOB by default
Hi, Markus, Marc-Andre,
I didn't follow up the latest discussions on the response queue or monitor changes. For now this series still makes sense to me, so I'm just rebasing the series to master and repost (with some additional changes/patches to address Markus's concerns). Please let me know if you have made any conclusion that I'm not aware of, or if this work needs any further rebasing.
v6 (previous version of the series might be incorrect; from this version, I continued to use the old version counts):
- collect r-bs
- use Markus's version of build_params() [Markus]
- use \' instead of \" in Python codes [Markus]
- in qapi_event_send_*() still pass in &error_abort instead of NULL [Markus]
- remove COMMAND_DROPPED from document too
- add missing pieces for test-qmp-cmds [Markus]
- add one patch to remove as much different code path for oob as possible [Markus]
TODO:
- flow control test is still missing; I still didn't consider it much, also we possibly need to touch up monitor_puts() to use limited sized buffer to complete the flow control of QMP channel
Please review. Thanks,
Markus Armbruster (1):
qapi: Fix build_params() for empty parameter list
Peter Xu (12):
monitor: simplify monitor_qmp_setup_handlers_bh
qapi: remove error checks for event emission
monitor: move need_resume flag into monitor struct
monitor: suspend monitor instead of send CMD_DROP
qapi: remove COMMAND_DROPPED event
monitor: restrict response queue length too
monitor: remove "x-oob", turn oob on by default
Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
monitor: add traces for qmp queues
tests: remove "0.15" prefix for test-qmp-cmds
tests: add oob functional test for test-qmp-cmds
monitor: reduce different code path for oob
block/block-backend.c | 8 +- block/qcow2.c | 2 +- block/quorum.c | 4 +- block/write-threshold.c | 3 +- blockjob.c | 13 +-- cpus.c | 8 +- docs/devel/qapi-code-gen.txt | 6 +- docs/interop/qmp-spec.txt | 5 +- dump.c | 3 +- hw/acpi/core.c | 2 +- hw/acpi/cpu.c | 2 +- hw/acpi/memory_hotplug.c | 5 +- hw/char/virtio-console.c | 3 +- hw/core/qdev.c | 3 +- hw/net/virtio-net.c | 2 +- hw/ppc/spapr_rtc.c | 2 +- hw/timer/mc146818rtc.c | 2 +- hw/virtio/virtio-balloon.c | 3 +- hw/watchdog/watchdog.c | 15 ++- include/monitor/monitor.h | 1 - include/qapi/qmp-event.h | 3 +- job.c | 2 +- migration/migration.c | 4 +- migration/ram.c | 2 +- monitor.c | 194 +++++++++++++++++++---------------- qapi/misc.json | 40 -------- scripts/qapi/common.py | 10 +- scripts/qapi/events.py | 23 ++--- scsi/pr-manager-helper.c | 3 +- tests/libqtest.c | 10 +- tests/libqtest.h | 4 +- tests/qmp-test.c | 6 +- tests/test-qmp-cmds.c | 26 ++++- tests/test-qmp-event.c | 11 +- trace-events | 4 + ui/spice-core.c | 10 +- ui/vnc.c | 7 +- vl.c | 21 ++-- 38 files changed, 210 insertions(+), 262 deletions(-)
Signature · 1 line
2.17.1
PATCH v6 1/13 monitor: simplify monitor_qmp_setup_handlers_bh
When we reach monitor_qmp_setup_handlers_bh() we must be using the IOThread then, so no need to check against it any more. Instead, we assert.
--- monitor.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
monitor.c +4 -9
@@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)Monitor *mon = opaque;GMainContext *context; -if (mon->use_io_thread) { -/* Use @mon_iothread context */ - context =monitor_get_io_context (); -assert (context); - }else { -/* Use default main loop context */ - context =NULL ; - } - +assert (mon->use_io_thread); +/* Use @mon_iothread context */ + context =monitor_get_io_context (); +assert (context);qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event,NULL , mon, context, true);monitor_list_append (mon);
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
Hi
Quoted 6 lines · Peter Xu
When we reach
monitor_qmp_setup_handlers_bh()we must be using the IOThread then, so no need to check against it any more. Instead, we assert.Reviewed-by Markus Armbruster <[email protected]>Signed-off-by Peter Xu <[email protected]>
That's a clear simplification that I also found, so ack in principle.
However, I don't understand the need of a BH in the first place. monitor_get_io_context() will return the iothread associated context. Could you explain?
thanks
Quoted 31 lines
--- monitor.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)monitor.c +4
-10@@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)Monitor *mon = opaque;GMainContext *context; -if (mon->use_io_thread) { -/* Use @mon_iothread context */ - context =monitor_get_io_context (); -assert (context); - }else { -/* Use default main loop context */ - context =NULL ; - } - +assert (mon->use_io_thread); +/* Use @mon_iothread context */ + context =monitor_get_io_context (); +assert (context);qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event,NULL , mon, context, true);monitor_list_append (mon); --2.17.1
Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
Quoted 10 lines · Marc-André Lureau
Hi
Quoted 6 lines · Peter Xu
When we reach
monitor_qmp_setup_handlers_bh()we must be using the IOThread then, so no need to check against it any more. Instead, we assert.Reviewed-by Markus Armbruster <[email protected]>Signed-off-by Peter Xu <[email protected]>
Quoted 6 lines
That's a clear simplification that I also found, so ack in principle.
However, I don't understand the need of a BH in the first place.
monitor_get_io_context()will return the iothread associated context. Could you explain?
thanks
Please refer to the comment in monitor_init():
/*
* We can't call qemu_chr_fe_set_handlers() directly here
* since chardev might be running in the monitor I/O
* thread. Schedule a bottom half.
*/ aio_bh_schedule_oneshot(monitor_get_aio_context(),
monitor_qmp_setup_handlers_bh, mon);
To be more specific: in qemu_chr_fe_set_handlers() after we call qemu_chr_be_update_read_handlers() then the iothread might already have started to operate on the chardev, so it can race with qemu_chr_fe_set_handlers() itself if we call it directly in the main thread.
Regards,
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
In reply to Peter XuQuoted 19 lines · Peter Xu
When we reach
monitor_qmp_setup_handlers_bh()we must be using the IOThread then, so no need to check against it any more. Instead, we assert.Reviewed-by Markus Armbruster <[email protected]>Signed-off-by Peter Xu <[email protected]>--- monitor.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)monitor.c +0
-1@@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)Monitor *mon = opaque;GMainContext *context; -if (mon->use_io_thread) {
monitor.c static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4628
Quoted 10 lines
monitor.c +2
-8-/* Use @mon_iothread context */ - context =monitor_get_io_context (); -assert (context); - }else { -/* Use default main loop context */ - context =NULL ; - } - +assert (mon->use_io_thread); +/* Use @mon_iothread context */
Mind if I drop this comment?
monitor.c static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4633
Quoted 5 lines
monitor.c +2
-0+ context =monitor_get_io_context (); +assert (context);qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event,NULL , mon, context, true);monitor_list_append (mon);
R-by stands, of course.
Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
Quoted 4 lines · Markus Armbruster
Quoted 4 lines
monitor.c +0
-0@@ -4624,15 +4624,10 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)Monitor *mon = opaque;GMainContext *context;
Quoted 13 lines
monitor.c static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4628
Quoted 11 lines
monitor.c +2
-9-if (mon->use_io_thread) { -/* Use @mon_iothread context */ - context =monitor_get_io_context (); -assert (context); - }else { -/* Use default main loop context */ - context =NULL ; - } - +assert (mon->use_io_thread); +/* Use @mon_iothread context */ Mind if I drop this comment?
Yes, please.
Quoted 8 lines
monitor.c static void monitor_qmp_setup_handlers_bh(void *opaque) · line 4633
Quoted 6 lines
monitor.c +2
-0+ context =monitor_get_io_context (); +assert (context);qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, monitor_qmp_event,NULL , mon, context, true);monitor_list_append (mon);R-by stands, of course.
Regards,
Signature · 1 line
Peter Xu
PATCH v6 2/13 qapi: Fix build_params() for empty parameter list
In reply to Peter XuFrom: Markus Armbruster <[email protected]>
build_params() returns '' instead of 'void' when there are no parameters. Can't happen now, but the next commit will change that.
--- scripts/qapi/common.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
scripts/qapi/common.py +4 -6
@@ -2067,16 +2067,14 @@ extern const QEnumLookup %(c_name)s_lookup; return ret -def build_params(arg_type, boxed, extra): - if not arg_type: - assert not boxed - return extra +def build_params(arg_type, boxed, extra=None): ret = '' sep = '' if boxed: + assert arg_type ret += '%s arg' % arg_type.c_param_type() sep = ', ' - else: + elif arg_type: assert not arg_type.variants for memb in arg_type.members: ret += sep
@@ -2087,7 +2085,7 @@ def build_params(arg_type, boxed, extra): c_name(memb.name)) if extra: ret += sep + extra - return ret + return ret if ret else 'void' #
Signature · 1 line
2.17.1
PATCH v6 3/13 qapi: remove error checks for event emission
In reply to Peter XuIn the whole QAPI event emission code we're passing in an Error* object along the whole stack. That's never useful since it never fails after all. Remove that.
Then, remove that parameter from all the callers to send an event.
--- block/block-backend.c | 8 +++----- block/qcow2.c | 2 +- block/quorum.c | 4 ++-- block/write-threshold.c | 3 +-- blockjob.c | 13 +++++-------- cpus.c | 8 ++++---- docs/devel/qapi-code-gen.txt | 6 ++---- dump.c | 3 +-- hw/acpi/core.c | 2 +- hw/acpi/cpu.c | 2 +- hw/acpi/memory_hotplug.c | 5 ++--- hw/char/virtio-console.c | 3 +-- hw/core/qdev.c | 3 +-- hw/net/virtio-net.c | 2 +- hw/ppc/spapr_rtc.c | 2 +- hw/timer/mc146818rtc.c | 2 +- hw/virtio/virtio-balloon.c | 3 +-- hw/watchdog/watchdog.c | 15 +++++++-------- include/qapi/qmp-event.h | 3 +-- job.c | 2 +- migration/migration.c | 4 ++-- migration/ram.c | 2 +- monitor.c | 5 ++--- scripts/qapi/events.py | 23 ++++++----------------- scsi/pr-manager-helper.c | 3 +-- tests/test-qmp-event.c | 11 +++++------ ui/spice-core.c | 10 ++++------ ui/vnc.c | 7 +++---- vl.c | 16 +++++++--------- 29 files changed, 69 insertions(+), 103 deletions(-)
block/block-backend.c +3 -5
@@ -981,8 +981,7 @@ void blk_dev_change_media_cb(BlockBackend *blk, bool load, Error **errp)if (tray_was_open != tray_is_open) {char *id =blk_get_attached_dev_id (blk); -qapi_event_send_device_tray_moved (blk_name (blk), id, tray_is_open, - &error_abort); +qapi_event_send_device_tray_moved (blk_name (blk), id, tray_is_open);g_free (id); } }
@@ -1666,8 +1665,7 @@ static void send_qmp_error_event(BlockBackend *blk,qapi_event_send_block_io_error (blk_name (blk), !!bs, bs ?bdrv_get_node_name (bs) :NULL , optype, action,blk_iostatus_is_enabled (blk), - error ==ENOSPC ,strerror (error), - &error_abort); + error ==ENOSPC ,strerror (error)); }/* This is done by device models because, while the block layer knows
@@ -1783,7 +1781,7 @@ void blk_eject(BlockBackend *blk, bool eject_flag)* the frontend experienced a tray event. */ id =blk_get_attached_dev_id (blk);qapi_event_send_device_tray_moved (blk_name (blk), id, - eject_flag, &error_abort); + eject_flag);g_free (id); }
block/qcow2.c +1 -1
@@ -4659,7 +4659,7 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset, *node_name !='\0' , node_name, message, offset >=0 , offset, size >=0 , size, - fatal, &error_abort); + fatal);g_free (message);if (fatal) {
block/quorum.c +2 -2
@@ -199,7 +199,7 @@ static void quorum_report_bad(QuorumOpType type, uint64_t offset, }qapi_event_send_quorum_report_bad (type, !!msg, msg, node_name, start_sector, - end_sector - start_sector, &error_abort); + end_sector - start_sector); }static void quorum_report_failure (QuorumAIOCB *acb)
@@ -210,7 +210,7 @@ static void quorum_report_failure(QuorumAIOCB *acb) BDRV_SECTOR_SIZE);qapi_event_send_quorum_failure (reference, start_sector, - end_sector - start_sector, &error_abort); + end_sector - start_sector); }static int quorum_vote_error (QuorumAIOCB *acb);
block/write-threshold.c +1 -2
@@ -63,8 +63,7 @@ static int coroutine_fn before_write_notify(NotifierWithReturn *notifier,qapi_event_send_block_write_threshold ( bs->node_name, amount, - bs->write_threshold_offset, - &error_abort); + bs->write_threshold_offset);/* autodisable to avoid flooding the monitor */ write_threshold_disable (bs);
blockjob.c +5 -8
@@ -315,8 +315,7 @@ static void block_job_event_cancelled(Notifier *n, void *opaque) job->job.id, job->job.progress_total, job->job.progress_current, - job->speed, - &error_abort); + job->speed); }static void block_job_event_completed (Notifier *n,void *opaque)
@@ -338,8 +337,7 @@ static void block_job_event_completed(Notifier *n, void *opaque) job->job.progress_current, job->speed, !!msg, - msg, - &error_abort); + msg); }static void block_job_event_pending (Notifier *n,void *opaque)
@@ -351,8 +349,7 @@ static void block_job_event_pending(Notifier *n, void *opaque) }qapi_event_send_block_job_pending (job_type (&job->job), - job->job.id, - &error_abort); + job->job.id); }static void block_job_event_ready (Notifier *n,void *opaque)
@@ -367,7 +364,7 @@ static void block_job_event_ready(Notifier *n, void *opaque) job->job.id, job->job.progress_total, job->job.progress_current, - job->speed, &error_abort); + job->speed); }
@@ -494,7 +491,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,qapi_event_send_block_job_error (job->job.id, is_read ?IO_OPERATION_TYPE_READ :IO_OPERATION_TYPE_WRITE , - action, &error_abort); + action); }if (action ==BLOCK_ERROR_ACTION_STOP ) {job_pause (&job->job);
cpus.c +4 -4
@@ -1011,7 +1011,7 @@ static int do_vm_stop(RunState state, bool send_stop)runstate_set (state);vm_state_notify (0 , state);if (send_stop) { -qapi_event_send_stop (&error_abort); +qapi_event_send_stop (); } }
@@ -2059,13 +2059,13 @@ int vm_prepare_start(void)* the STOP event. */ if (runstate_is_running ()) { -qapi_event_send_stop (&error_abort); -qapi_event_send_resume (&error_abort); +qapi_event_send_stop (); +qapi_event_send_resume ();return -1 ; }/* We are sending this now, but the CPUs will be resumed shortly later */ -qapi_event_send_resume (&error_abort); +qapi_event_send_resume ();replay_enable_events ();cpu_enable_ticks ();
docs/devel/qapi-code-gen.txt +2 -4
@@ -1356,10 +1356,9 @@ Example: $ cat qapi-generated/example-qapi-events.c [Uninteresting stuff omitted...] - void qapi_event_send_my_event(Error **errp) + void qapi_event_send_my_event(void) { QDict *qmp; - Error *err = NULL; QMPEventFuncEmit emit; emit = qmp_event_get_func_emit();
@@ -1369,9 +1368,8 @@ Example: qmp = qmp_event_build_dict("MY_EVENT"); - emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err); + emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp); - error_propagate(errp, err); qobject_unref(qmp); }
dump.c +1 -2
@@ -1890,8 +1890,7 @@ static void dump_process(DumpState *s, Error **errp)/* should never fail */ assert (result);qapi_event_send_dump_completed (result, !!local_err, (local_err ? \ -error_get_pretty (local_err) :NULL ), - &error_abort); +error_get_pretty (local_err) :NULL ));qapi_free_DumpQueryResult (result);error_propagate (errp, local_err);
hw/acpi/core.c +1 -1
@@ -570,7 +570,7 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)break ;default :if (sus_typ == ar->pm1.cnt.s4_val) {/* S4 request */ -qapi_event_send_suspend_disk (&error_abort); +qapi_event_send_suspend_disk ();qemu_system_shutdown_request (SHUTDOWN_CAUSE_GUEST_SHUTDOWN ); }break ;
hw/acpi/cpu.c +1 -1
@@ -160,7 +160,7 @@ static void cpu_hotplug_wr(void *opaque, hwaddr addr, uint64_t data, cdev = &cpu_st->devs[cpu_st->selector]; cdev->ost_status = data; info =acpi_cpu_device_status (cpu_st->selector, cdev); -qapi_event_send_acpi_device_ost (info, &error_abort); +qapi_event_send_acpi_device_ost (info);qapi_free_ACPIOSTInfo (info);trace_cpuhp_acpi_write_ost_status (cpu_st->selector, cdev->ost_status);
hw/acpi/memory_hotplug.c +2 -3
@@ -161,7 +161,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,/* TODO: implement memory removal on guest signal */ info =acpi_memory_device_status (mem_st->selector, mdev); -qapi_event_send_acpi_device_ost (info, &error_abort); +qapi_event_send_acpi_device_ost (info);qapi_free_ACPIOSTInfo (info);break ;case 0x14 :/* set is_* fields */
@@ -185,8 +185,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,if (local_err) {trace_mhp_acpi_pc_dimm_delete_failed (mem_st->selector);qapi_event_send_mem_unplug_error (dev->id, -error_get_pretty (local_err), - &error_abort); +error_get_pretty (local_err));error_free (local_err);break ; }
hw/char/virtio-console.c +1 -2
@@ -114,8 +114,7 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected) }if (dev->id) { -qapi_event_send_vserport_change (dev->id, guest_connected, - &error_abort); +qapi_event_send_vserport_change (dev->id, guest_connected); } }
hw/core/qdev.c +1 -2
@@ -1000,8 +1000,7 @@ static void device_finalize(Object *obj)if (dev->pending_deleted_event) {g_assert (dev->canonical_path); -qapi_event_send_device_deleted (!!dev->id, dev->id, dev->canonical_path, - &error_abort); +qapi_event_send_device_deleted (!!dev->id, dev->id, dev->canonical_path);g_free (dev->canonical_path); dev->canonical_path =NULL ; }
hw/net/virtio-net.c +1 -1
@@ -329,7 +329,7 @@ static void rxfilter_notify(NetClientState *nc)if (nc->rxfilter_notify_enabled) {gchar *path =object_get_canonical_path (OBJECT (n->qdev));qapi_event_send_nic_rx_filter_changed (!!n->netclient_name, - n->netclient_name, path, &error_abort); + n->netclient_name, path);g_free (path);/* disable event notification to avoid events flooding */
hw/ppc/spapr_rtc.c +1 -1
@@ -118,7 +118,7 @@ static void rtas_set_time_of_day(PowerPCCPU *cpu, sPAPRMachineState *spapr, }/* Generate a monitor event for the change */ -qapi_event_send_rtc_change (qemu_timedate_diff (&tm), &error_abort); +qapi_event_send_rtc_change (qemu_timedate_diff (&tm)); host_ns =qemu_clock_get_ns (rtc_clock);
hw/timer/mc146818rtc.c +1 -1
@@ -635,7 +635,7 @@ static void rtc_set_time(RTCState *s) s->base_rtc =mktimegm (&tm); s->last_update =qemu_clock_get_ns (rtc_clock); -qapi_event_send_rtc_change (qemu_timedate_diff (&tm), &error_abort); +qapi_event_send_rtc_change (qemu_timedate_diff (&tm)); }static void rtc_set_cmos (RTCState *s,const struct tm *tm)
hw/virtio/virtio-balloon.c +1 -2
@@ -367,8 +367,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev, dev->actual =le32_to_cpu (config.actual);if (dev->actual != oldactual) {qapi_event_send_balloon_change (vm_ram_size - - ((ram_addr_t ) dev->actual <<VIRTIO_BALLOON_PFN_SHIFT ), - &error_abort); + ((ram_addr_t ) dev->actual <<VIRTIO_BALLOON_PFN_SHIFT )); }trace_virtio_balloon_set_config (dev->actual, oldactual); }
hw/watchdog/watchdog.c +7 -8
@@ -102,17 +102,17 @@ void watchdog_perform_action(void) {switch (watchdog_action) {case WATCHDOG_ACTION_RESET :/* same as 'system_reset' in monitor */ -qapi_event_send_watchdog (WATCHDOG_ACTION_RESET , &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_RESET );qemu_system_reset_request (SHUTDOWN_CAUSE_GUEST_RESET );break ;case WATCHDOG_ACTION_SHUTDOWN :/* same as 'system_powerdown' in monitor */ -qapi_event_send_watchdog (WATCHDOG_ACTION_SHUTDOWN , &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_SHUTDOWN );qemu_system_powerdown_request ();break ;case WATCHDOG_ACTION_POWEROFF :/* same as 'quit' command in monitor */ -qapi_event_send_watchdog (WATCHDOG_ACTION_POWEROFF , &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_POWEROFF );exit (0 );case WATCHDOG_ACTION_PAUSE :/* same as 'stop' command in monitor */
@@ -120,22 +120,21 @@ void watchdog_perform_action(void)* you would get a deadlock. Bypass the problem. */ qemu_system_vmstop_request_prepare (); -qapi_event_send_watchdog (WATCHDOG_ACTION_PAUSE , &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_PAUSE );qemu_system_vmstop_request (RUN_STATE_WATCHDOG );break ;case WATCHDOG_ACTION_DEBUG : -qapi_event_send_watchdog (WATCHDOG_ACTION_DEBUG , &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_DEBUG );fprintf (stderr,"watchdog: timer fired\n" );break ;case WATCHDOG_ACTION_NONE : -qapi_event_send_watchdog (WATCHDOG_ACTION_NONE , &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_NONE );break ;case WATCHDOG_ACTION_INJECT_NMI : -qapi_event_send_watchdog (WATCHDOG_ACTION_INJECT_NMI , - &error_abort); +qapi_event_send_watchdog (WATCHDOG_ACTION_INJECT_NMI );nmi_monitor_handle (0 ,NULL );break ;
include/qapi/qmp-event.h +1 -2
@@ -14,8 +14,7 @@#ifndef QMP_EVENT_H #define QMP_EVENT_H - -typedef void (*QMPEventFuncEmit )(unsigned event,QDict *dict,Error **errp); +typedef void (*QMPEventFuncEmit )(unsigned event,QDict *dict);void qmp_event_set_func_emit (QMPEventFuncEmit emit);
job.c +1 -1
@@ -174,7 +174,7 @@ static void job_state_transition(Job *job, JobStatus s1) job->status = s1;if (!job_is_internal (job) && s1 != s0) { -qapi_event_send_job_status_change (job->id, job->status, &error_abort); +qapi_event_send_job_status_change (job->id, job->status); } }
migration/migration.c +2 -2
@@ -203,7 +203,7 @@ void migration_incoming_state_destroy(void)static void migrate_generate_event (int new_state) {if (migrate_use_events ()) { -qapi_event_send_migration (new_state, &error_abort); +qapi_event_send_migration (new_state); } }
@@ -301,7 +301,7 @@ void qemu_start_incoming_migration(const char *uri, Error **errp) {const char *p; -qapi_event_send_migration (MIGRATION_STATUS_SETUP , &error_abort); +qapi_event_send_migration (MIGRATION_STATUS_SETUP );if (!strcmp (uri,"defer" )) {deferred_incoming_migration (errp); }else if (strstart (uri,"tcp:" , &p)) {
migration/ram.c +1 -1
@@ -1660,7 +1660,7 @@ static void migration_bitmap_sync(RAMState *rs) rs->bytes_xfer_prev = bytes_xfer_now; }if (migrate_use_events ()) { -qapi_event_send_migration_pass (ram_counters.dirty_sync_count,NULL ); +qapi_event_send_migration_pass (ram_counters.dirty_sync_count); } }
monitor.c +2 -3
@@ -689,7 +689,7 @@ monitor_qapi_event_queue_no_reenter(QAPIEvent event, QDict *qdict) }static void -monitor_qapi_event_queue (QAPIEvent event,QDict *qdict,Error **errp) +monitor_qapi_event_queue (QAPIEvent event,QDict *qdict) {/* * monitor_qapi_event_queue_no_reenter() is not reentrant: it
@@ -4310,8 +4310,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)* that command was dropped. */ qapi_event_send_command_dropped (id, -COMMAND_DROP_REASON_QUEUE_FULL , - &error_abort); +COMMAND_DROP_REASON_QUEUE_FULL );qmp_request_free (req_obj);return ; }
scripts/qapi/events.py +6 -17
@@ -18,7 +18,7 @@ from qapi.common import * def build_event_send_proto(name, arg_type, boxed): return 'void qapi_event_send_%(c_name)s(%(param)s)' % { 'c_name': c_name(name.lower()), - 'param': build_params(arg_type, boxed, 'Error **errp')} + 'param': build_params(arg_type, boxed)} def gen_event_send_decl(name, arg_type, boxed):
@@ -70,7 +70,6 @@ def gen_event_send(name, arg_type, boxed, event_enum_name): %(proto)s { QDict *qmp; - Error *err = NULL; QMPEventFuncEmit emit; ''', proto=build_event_send_proto(name, arg_type, boxed))
@@ -103,45 +102,35 @@ def gen_event_send(name, arg_type, boxed, event_enum_name): ''') if not arg_type.is_implicit(): ret += mcgen(''' - visit_type_%(c_name)s(v, "%(name)s", &arg, &err); + visit_type_%(c_name)s(v, "%(name)s", &arg, &error_abort); ''', name=name, c_name=arg_type.c_name()) else: ret += mcgen(''' - visit_start_struct(v, "%(name)s", NULL, 0, &err); - if (err) { - goto out; - } - visit_type_%(c_name)s_members(v, ¶m, &err); - if (!err) { - visit_check_struct(v, &err); - } + visit_start_struct(v, "%(name)s", NULL, 0, &error_abort); + visit_type_%(c_name)s_members(v, ¶m, &error_abort); + visit_check_struct(v, &error_abort); visit_end_struct(v, NULL); ''', name=name, c_name=arg_type.c_name()) ret += mcgen(''' - if (err) { - goto out; - } visit_complete(v, &obj); qdict_put_obj(qmp, "data", obj); ''') ret += mcgen(''' - emit(%(c_enum)s, qmp, &err); + emit(%(c_enum)s, qmp); ''', c_enum=c_enum_const(event_enum_name, name)) if arg_type and not arg_type.is_empty(): ret += mcgen(''' -out: visit_free(v); ''') ret += mcgen(''' - error_propagate(errp, err); qobject_unref(qmp); } ''')
scsi/pr-manager-helper.c +1 -2
@@ -44,8 +44,7 @@ static void pr_manager_send_status_changed_event(PRManagerHelper *pr_mgr)char *id =object_get_canonical_path_component (OBJECT (pr_mgr));if (id) { -qapi_event_send_pr_manager_status_changed (id, !!pr_mgr->ioc, - &error_abort); +qapi_event_send_pr_manager_status_changed (id, !!pr_mgr->ioc);g_free (id); } }
tests/test-qmp-event.c +5 -6
@@ -95,7 +95,7 @@ static bool qdict_cmp_simple(QDict *a, QDict *b)/* This function is hooked as final emit function, which can verify the correctness. */ -static void event_test_emit (test_QAPIEvent event,QDict *d,Error **errp) +static void event_test_emit (test_QAPIEvent event,QDict *d) {QDict *t;int64_t s, ms;
@@ -156,7 +156,7 @@ static void test_event_a(TestEventData *data,QDict *d; d = data->expect;qdict_put_str (d,"event" ,"EVENT_A" ); -qapi_event_send_event_a (&error_abort); +qapi_event_send_event_a (); } static void test_event_b(TestEventData *data,
@@ -165,7 +165,7 @@ static void test_event_b(TestEventData *data,QDict *d; d = data->expect;qdict_put_str (d,"event" ,"EVENT_B" ); -qapi_event_send_event_b (&error_abort); +qapi_event_send_event_b (); } static void test_event_c(TestEventData *data,
@@ -191,7 +191,7 @@ static void test_event_c(TestEventData *data,qdict_put_str (d,"event" ,"EVENT_C" );qdict_put (d,"data" , d_data); -qapi_event_send_event_c (true,1 , true, &b,"test2" , &error_abort); +qapi_event_send_event_c (true,1 , true, &b,"test2" );g_free (b.string); }
@@ -233,8 +233,7 @@ static void test_event_d(TestEventData *data,qdict_put_str (d,"event" ,"EVENT_D" );qdict_put (d,"data" , d_data); -qapi_event_send_event_d (&a,"test3" , false,NULL , true,ENUM_ONE_VALUE3 , - &error_abort); +qapi_event_send_event_d (&a,"test3" , false,NULL , true,ENUM_ONE_VALUE3 );g_free (struct1.string);g_free (a.string);
ui/spice-core.c +4 -6
@@ -218,8 +218,7 @@ static void channel_event(int event, SpiceChannelEventInfo *info)switch (event) {case SPICE_CHANNEL_EVENT_CONNECTED :qapi_event_send_spice_connected (qapi_SpiceServerInfo_base (server), -qapi_SpiceChannel_base (client), - &error_abort); +qapi_SpiceChannel_base (client));break ;case SPICE_CHANNEL_EVENT_INITIALIZED :if (auth) {
@@ -228,13 +227,12 @@ static void channel_event(int event, SpiceChannelEventInfo *info) }add_channel_info (client, info);channel_list_add (info); -qapi_event_send_spice_initialized (server, client, &error_abort); +qapi_event_send_spice_initialized (server, client);break ;case SPICE_CHANNEL_EVENT_DISCONNECTED :channel_list_del (info);qapi_event_send_spice_disconnected (qapi_SpiceServerInfo_base (server), -qapi_SpiceChannel_base (client), - &error_abort); +qapi_SpiceChannel_base (client));break ;default :break ;
@@ -287,7 +285,7 @@ static void migrate_connect_complete_cb(SpiceMigrateInstance *sin)static void migrate_end_complete_cb (SpiceMigrateInstance *sin) { -qapi_event_send_spice_migrate_completed (&error_abort); +qapi_event_send_spice_migrate_completed (); spice_migration_completed = true; }
ui/vnc.c +3 -4
@@ -296,14 +296,13 @@ static void vnc_qmp_event(VncState *vs, QAPIEvent event)switch (event) {case QAPI_EVENT_VNC_CONNECTED : -qapi_event_send_vnc_connected (si,qapi_VncClientInfo_base (vs->info), - &error_abort); +qapi_event_send_vnc_connected (si,qapi_VncClientInfo_base (vs->info));break ;case QAPI_EVENT_VNC_INITIALIZED : -qapi_event_send_vnc_initialized (si, vs->info, &error_abort); +qapi_event_send_vnc_initialized (si, vs->info);break ;case QAPI_EVENT_VNC_DISCONNECTED : -qapi_event_send_vnc_disconnected (si, vs->info, &error_abort); +qapi_event_send_vnc_disconnected (si, vs->info);break ;default :break ;
vl.c +7 -9
@@ -1647,8 +1647,7 @@ void qemu_system_reset(ShutdownCause reason)qemu_devices_reset (); }if (reason !=SHUTDOWN_CAUSE_SUBSYSTEM_RESET ) { -qapi_event_send_reset (shutdown_caused_by_guest (reason), - &error_abort); +qapi_event_send_reset (shutdown_caused_by_guest (reason)); }cpu_synchronize_all_post_reset (); }
@@ -1661,11 +1660,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) current_cpu->crash_occurred = true; }qapi_event_send_guest_panicked (GUEST_PANIC_ACTION_PAUSE , - !!info, info, &error_abort); + !!info, info);vm_stop (RUN_STATE_GUEST_PANICKED );if (!no_shutdown) {qapi_event_send_guest_panicked (GUEST_PANIC_ACTION_POWEROFF , - !!info, info, &error_abort); + !!info, info);qemu_system_shutdown_request (SHUTDOWN_CAUSE_GUEST_PANIC ); }
@@ -1706,7 +1705,7 @@ static void qemu_system_suspend(void)pause_all_vcpus ();notifier_list_notify (&suspend_notifiers,NULL );runstate_set (RUN_STATE_SUSPENDED ); -qapi_event_send_suspend (&error_abort); +qapi_event_send_suspend (); }void qemu_system_suspend_request (void )
@@ -1776,7 +1775,7 @@ void qemu_system_shutdown_request(ShutdownCause reason)static void qemu_system_powerdown (void ) { -qapi_event_send_powerdown (&error_abort); +qapi_event_send_powerdown ();notifier_list_notify (&powerdown_notifiers,NULL ); }
@@ -1819,8 +1818,7 @@ static bool main_loop_should_exit(void) request =qemu_shutdown_requested ();if (request) {qemu_kill_report (); -qapi_event_send_shutdown (shutdown_caused_by_guest (request), - &error_abort); +qapi_event_send_shutdown (shutdown_caused_by_guest (request));if (no_shutdown) {vm_stop (RUN_STATE_SHUTDOWN ); }else {
@@ -1843,7 +1841,7 @@ static bool main_loop_should_exit(void)notifier_list_notify (&wakeup_notifiers, &wakeup_reason); wakeup_reason =QEMU_WAKEUP_REASON_NONE ;resume_all_vcpus (); -qapi_event_send_wakeup (&error_abort); +qapi_event_send_wakeup (); }if (qemu_powerdown_requested ()) {qemu_system_powerdown ();
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
Quoted 3 lines · Peter Xu
In the whole QAPI event emission code we're passing in an Error* object along the whole stack. That's never useful since it never fails after all. Remove that.
This is the interesting part. We'll see below why it can't fail.
Quoted 1 line
Then, remove that parameter from all the callers to send an event.
This is the mechanical part. The callers pass &error_abort, except for one that passes NULL.
The patch moves the &error_abort argument from the qapi_event_send_FOO() calls to the places where the argument is used. No effect except for the caller that passes NULL. That one now asserts "no error".
Quoted 4 lines
Suggested-by Eric Blake <[email protected]>Suggested-by Markus Armbruster <[email protected]>Signed-off-by Peter Xu <[email protected]>---
Patch quoted except for the parts that drop &error_abort from qapi_event_send_FOO() calls:
[...]
Quoted 27 lines
docs/devel/qapi-code-gen.txt +2
-4@@ -1356,10 +1356,9 @@ Example: $ cat qapi-generated/example-qapi-events.c [Uninteresting stuff omitted...] - void qapi_event_send_my_event(Error **errp) + void qapi_event_send_my_event(void) { QDict *qmp; - Error *err = NULL; QMPEventFuncEmit emit; emit = qmp_event_get_func_emit();@@ -1369,9 +1368,8 @@ Example: qmp = qmp_event_build_dict("MY_EVENT"); - emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err); + emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp); - error_propagate(errp, err); qobject_unref(qmp); }
[...]
Quoted 14 lines
include/qapi/qmp-event.h +1
-2@@ -14,8 +14,7 @@#ifndef QMP_EVENT_H #define QMP_EVENT_H - -typedef void (*QMPEventFuncEmit )(unsigned event,QDict *dict,Error **errp); +typedef void (*QMPEventFuncEmit )(unsigned event,QDict *dict);void qmp_event_set_func_emit (QMPEventFuncEmit emit);
[...]
migration/ram.c static void migration_bitmap_sync(RAMState *rs) · line 1676
Quoted 13 lines
migration/ram.c +1
-1@@ -1670,7 +1670,7 @@ static void migration_bitmap_sync(RAMState *rs) rs->bytes_xfer_prev = bytes_xfer_now; }if (migrate_use_events ()) { -qapi_event_send_migration_pass (ram_counters.dirty_sync_count,NULL ); +qapi_event_send_migration_pass (ram_counters.dirty_sync_count); } }
This is the one caller that ignores errors instead of asserting they can't happen.
monitor.c monitor_qapi_event_queue_no_reenter(QAPIEvent event, QDict *qdict) · line 695
Quoted 13 lines
monitor.c +1
-1@@ -689,7 +689,7 @@ monitor_qapi_event_queue_no_reenter(QAPIEvent event, QDict *qdict) }static void -monitor_qapi_event_queue (QAPIEvent event,QDict *qdict,Error **errp) +monitor_qapi_event_queue (QAPIEvent event,QDict *qdict) {/* * monitor_qapi_event_queue_no_reenter() is not reentrant: it
Doesn't use its @errp argument, i.e. it can't fail.
[...]
scripts/qapi/events.py def gen_event_send(name, arg_type, boxed, event_enum_name):
Quoted 14 lines
scripts/qapi/events.py +1
-1@@ -18,7 +18,7 @@ from qapi.common import * def build_event_send_proto(name, arg_type, boxed): return 'void qapi_event_send_%(c_name)s(%(param)s)' % { 'c_name': c_name(name.lower()), - 'param': build_params(arg_type, boxed, 'Error **errp')} + 'param': build_params(arg_type, boxed)} def gen_event_send_decl(name, arg_type, boxed):@@ -70,7 +70,6 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
This is where we change to &error_abort. We need to show "can't fail".
scripts/qapi/events.py def gen_event_send(name, arg_type, boxed, event_enum_name):
Quoted 8 lines
scripts/qapi/events.py +0
-1%(proto)s { QDict *qmp; - Error *err = NULL; QMPEventFuncEmit emit; ''', proto=build_event_send_proto(name, arg_type, boxed))@@ -103,45 +102,35 @@ def gen_event_send(name, arg_type, boxed, event_enum_name):
if arg_type and not arg_type.is_empty():
ret += mcgen('''
v = qobject_output_visitor_new(&obj);scripts/qapi/events.py def gen_event_send(name, arg_type, boxed, event_enum_name): · line 105
Quoted 5 lines
scripts/qapi/events.py +1
-1''') if not arg_type.is_implicit(): ret += mcgen(''' - visit_type_%(c_name)s(v, "%(name)s", &arg, &err); + visit_type_%(c_name)s(v, "%(name)s", &arg, &error_abort);
Correct, because the QObject output visitor never fails.
Quoted 6 lines
scripts/qapi/events.py +0
-1''', name=name, c_name=arg_type.c_name()) else: ret += mcgen(''' - visit_start_struct(v, "%(name)s", NULL, 0, &err);
scripts/qapi/events.py def gen_event_send(name, arg_type, boxed, event_enum_name): · line 113
Quoted 10 lines
scripts/qapi/events.py +3
-7- if (err) { - goto out; - } - visit_type_%(c_name)s_members(v, ¶m, &err); - if (!err) { - visit_check_struct(v, &err); - } + visit_start_struct(v, "%(name)s", NULL, 0, &error_abort); + visit_type_%(c_name)s_members(v, ¶m, &error_abort); + visit_check_struct(v, &error_abort);
Likewise.
Quoted 5 lines
scripts/qapi/events.py +0
-1visit_end_struct(v, NULL); ''', name=name, c_name=arg_type.c_name()) ret += mcgen(''' - if (err) {
scripts/qapi/events.py def gen_event_send(name, arg_type, boxed, event_enum_name): · line 124
Quoted 10 lines
scripts/qapi/events.py +1
-3- goto out; - } visit_complete(v, &obj); qdict_put_obj(qmp, "data", obj); ''') ret += mcgen(''' - emit(%(c_enum)s, qmp, &err); + emit(%(c_enum)s, qmp);
@emit is either monitor_qapi_event_queue() or event_test_emit(). Neither can fail.
Quoted 14 lines
scripts/qapi/events.py +0
-2''', c_enum=c_enum_const(event_enum_name, name)) if arg_type and not arg_type.is_empty(): ret += mcgen(''' -out: visit_free(v); ''') ret += mcgen(''' - error_propagate(errp, err); qobject_unref(qmp); } ''')
[...]
tests/test-qmp-event.c static bool qdict_cmp_simple(QDict *a, QDict *b) · line 101
Quoted 13 lines
tests/test-qmp-event.c +1
-1@@ -95,7 +95,7 @@ static bool qdict_cmp_simple(QDict *a, QDict *b)/* This function is hooked as final emit function, which can verify the correctness. */ -static void event_test_emit (test_QAPIEvent event,QDict *d,Error **errp) +static void event_test_emit (test_QAPIEvent event,QDict *d) {QDict *t;int64_t s, ms;
Doesn't use its @errp argument, i.e. it can't fail.
[...]
Let's improve the commit message a bit. Here's my try:
qapi: Drop qapi_event_send_FOO()'s Error ** argument
The generated qapi_event_send_FOO() take an Error ** argument. They can't actually fail, because all they do with the argument is passing it to functions that can't fail: the QObject output visitor, and the @qmp_emit callback, which is either monitor_qapi_event_queue() or event_test_emit().
Drop the argument, and pass &error_abort to the QObject output visitor and @qmp_emit instead.
With something like that:
Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
On Mon, Aug 27, 2018 at 02:14:25PM +0200, Markus Armbruster wrote:
[...]
Quoted 4 lines
Let's improve the commit message a bit. Here's my try:
qapi: Drop qapi_event_send_FOO()'s Error ** argument
Quoted 8 lines
The generated
qapi_event_send_FOO()take an Error ** argument. They can't actually fail, because all they do with the argument is passing it to functions that can't fail: the QObject output visitor, and the @qmp_emit callback, which is eithermonitor_qapi_event_queue()orevent_test_emit().Drop the argument, and pass
&error_abortto the QObject output visitor and @qmp_emit instead.
I'm stealing this into my local tree.
Quoted 3 lines
With something like that:
Reviewed-by Markus Armbruster <[email protected]>
Thanks for the analysis and r-b; stealing the r-b too.
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
In reply to Markus ArmbrusterQuoted 22 lines · Markus Armbruster
Quoted 3 lines · Peter Xu
In the whole QAPI event emission code we're passing in an Error* object along the whole stack. That's never useful since it never fails after all. Remove that.
This is the interesting part. We'll see below why it can't fail.
Quoted 1 line
Then, remove that parameter from all the callers to send an event.
This is the mechanical part. The callers pass
&error_abort, except for one that passes NULL.The patch moves the
&error_abortargument from theqapi_event_send_FOO()calls to the places where the argument is used. No effect except for the caller that passes NULL. That one now asserts "no error".Quoted 4 lines
Suggested-by Eric Blake <[email protected]>Suggested-by Markus Armbruster <[email protected]>Signed-off-by Peter Xu <[email protected]>---
Quoted 8 lines
Patch quoted except for the parts that drop
&error_abortfromqapi_event_send_FOO()calls:[...]
docs/devel/qapi-code-gen.txt
Quoted 4 lines
docs/devel/qapi-code-gen.txt +0
-0
Missing:
@@ -1340,7 +1340,7 @@ Example:
#include "example-qapi-types.h"
- void qapi_event_send_my_event(Error **errp);
+ void qapi_event_send_my_event(void);
typedef enum example_QAPIEvent {
EXAMPLE_QAPI_EVENT_MY_EVENT = 0,
Quoted 23 lines
Quoted 23 lines
docs/devel/qapi-code-gen.txt +2
-4@@ -1356,10 +1356,9 @@ Example: $ cat qapi-generated/example-qapi-events.c [Uninteresting stuff omitted...] - void qapi_event_send_my_event(Error **errp) + void qapi_event_send_my_event(void) { QDict *qmp; - Error *err = NULL; QMPEventFuncEmit emit; emit = qmp_event_get_func_emit();@@ -1369,9 +1368,8 @@ Example: qmp = qmp_event_build_dict("MY_EVENT"); - emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp, &err); + emit(EXAMPLE_QAPI_EVENT_MY_EVENT, qmp); - error_propagate(errp, err); qobject_unref(qmp); }
[...]
PATCH v6 4/13 monitor: move need_resume flag into monitor struct
In reply to Peter XuIt was put into the request object to show whether we'll need to resume the monitor after dispatching the command. Now we move that into the monitor struct so that it might be even used in other places in the future, e.g., out-of-band message flow controls.
One thing to mention is that there is no lock needed before when accessing the flag since the request object will always be owned by a single thread. After we move it into monitor struct we need to protect that flag since it might be accessed by multiple threads now. Renaming the qmp_queue_lock into qmp_lock to protect the flag as well.
No functional change.
--- monitor.c | 72 +++++++++++++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 31 deletions(-)
monitor.c +41 -31
@@ -177,14 +177,20 @@ typedef struct {bool capab_offered[QMP_CAPABILITY__MAX ];/* capabilities offered */ bool capab[QMP_CAPABILITY__MAX ];/* offered and accepted */ /* -* Protects qmp request/response queue. +* Protects qmp request/response queue, and need_resume flag. * Take monitor_lock first when you need both. */ -QemuMutex qmp_queue_lock; +QemuMutex qmp_lock;/* Input queue that holds all the parsed QMP requests */ GQueue *qmp_requests;/* Output queue contains all the QMP responses in order */ GQueue *qmp_responses; +/* +* Whether we need to resume the monitor afterward. This flag is +* used to emulate the old QMP server behavior that the current +* command must be completed before execution of the next one. +*/ +bool need_resume; } MonitorQMP;/*
@@ -262,12 +268,6 @@ struct QMPRequest {*/ QObject *req;Error *err; -/* -* Whether we need to resume the monitor afterward. This flag is -* used to emulate the old QMP server behavior that the current -* command must be completed before execution of the next one. -*/ -bool need_resume; };typedef struct QMPRequest QMPRequest ;
@@ -368,7 +368,7 @@ static void qmp_request_free(QMPRequest *req)g_free (req); } -/* Caller must hold mon->qmp.qmp_queue_lock */ +/* Caller must hold mon->qmp.qmp_lock */ static void monitor_qmp_cleanup_req_queue_locked (Monitor *mon) {while (!g_queue_is_empty (mon->qmp.qmp_requests)) {
@@ -376,7 +376,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon) } } -/* Caller must hold the mon->qmp.qmp_queue_lock */ +/* Caller must hold the mon->qmp.qmp_lock */ static void monitor_qmp_cleanup_resp_queue_locked (Monitor *mon) {while (!g_queue_is_empty (mon->qmp.qmp_responses)) {
@@ -386,12 +386,23 @@ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)static void monitor_qmp_cleanup_queues (Monitor *mon) { -qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +qemu_mutex_lock (&mon->qmp.qmp_lock);monitor_qmp_cleanup_req_queue_locked (mon);monitor_qmp_cleanup_resp_queue_locked (mon); -qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_mutex_unlock (&mon->qmp.qmp_lock); } +/* Try to resume the monitor if it was suspended due to any reason */ +static void monitor_qmp_try_resume (Monitor *mon) +{ +assert (monitor_is_qmp (mon)); +qemu_mutex_lock (&mon->qmp.qmp_lock); +if (mon->qmp.need_resume) { +monitor_resume (mon); + mon->qmp.need_resume = false; + } +qemu_mutex_unlock (&mon->qmp.qmp_lock); +}static void monitor_flush_locked (Monitor *mon);
@@ -526,9 +537,9 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)* Push a reference to the response queue. The I/O thread * drains that queue and emits. */ -qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +qemu_mutex_lock (&mon->qmp.qmp_lock);g_queue_push_tail (mon->qmp.qmp_responses,qobject_ref (rsp)); -qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_mutex_unlock (&mon->qmp.qmp_lock);qemu_bh_schedule (qmp_respond_bh); }else {/*
@@ -549,9 +560,9 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon) {QDict *data; -qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +qemu_mutex_lock (&mon->qmp.qmp_lock); data =g_queue_pop_head (mon->qmp.qmp_responses); -qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_mutex_unlock (&mon->qmp.qmp_lock);return data; }
@@ -812,7 +823,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush, {memset (mon,0 ,sizeof (Monitor));qemu_mutex_init (&mon->mon_lock); -qemu_mutex_init (&mon->qmp.qmp_queue_lock); +qemu_mutex_init (&mon->qmp.qmp_lock); mon->outbuf =qstring_new ();/* Use *mon_cmds by default. */ mon->cmd_table = mon_cmds;
@@ -832,7 +843,7 @@ static void monitor_data_destroy(Monitor *mon)readline_free (mon->rs);qobject_unref (mon->outbuf);qemu_mutex_destroy (&mon->mon_lock); -qemu_mutex_destroy (&mon->qmp.qmp_queue_lock); +qemu_mutex_destroy (&mon->qmp.qmp_lock);monitor_qmp_cleanup_req_queue_locked (mon);monitor_qmp_cleanup_resp_queue_locked (mon);g_queue_free (mon->qmp.qmp_requests);
@@ -4191,9 +4202,9 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)qemu_mutex_lock (&monitor_lock); QTAILQ_FOREACH(mon, &mon_list, entry) { -qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +qemu_mutex_lock (&mon->qmp.qmp_lock); req_obj =g_queue_pop_head (mon->qmp.qmp_requests); -qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_mutex_unlock (&mon->qmp.qmp_lock);if (req_obj) {break ; }
@@ -4216,27 +4227,27 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)static void monitor_qmp_bh_dispatcher (void *data) {QMPRequest *req_obj =monitor_qmp_requests_pop_any (); +Monitor *mon;QDict *rsp;if (!req_obj) {return ; } + mon = req_obj->mon;if (req_obj->req) {trace_monitor_qmp_cmd_in_band (qobject_get_try_str (req_obj->id) ?:"" ); -monitor_qmp_dispatch (req_obj->mon, req_obj->req, req_obj->id); +monitor_qmp_dispatch (mon, req_obj->req, req_obj->id); }else {assert (req_obj->err); rsp =qmp_error_response (req_obj->err); req_obj->err =NULL ; -monitor_qmp_respond (req_obj->mon, rsp,NULL ); +monitor_qmp_respond (mon, rsp,NULL );qobject_unref (rsp); } -if (req_obj->need_resume) { -/* Pairs with the monitor_suspend() in handle_qmp_command() */ -monitor_resume (req_obj->mon); - } +monitor_qmp_try_resume (mon); +qmp_request_free (req_obj);/* Reschedule instead of looping so the main loop stays responsive */
@@ -4285,10 +4296,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) req_obj->id = id; req_obj->req = req; req_obj->err = err; - req_obj->need_resume = false;/* Protect qmp_requests and fetching its length. */ -qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +qemu_mutex_lock (&mon->qmp.qmp_lock);/* * If OOB is not enabled on the current monitor, we'll emulate the
@@ -4298,11 +4308,11 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)*/ if (!qmp_oob_enabled (mon)) {monitor_suspend (mon); - req_obj->need_resume = true; + mon->qmp.need_resume = true; }else {/* Drop the request if queue is full. */ if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX ) { -qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_mutex_unlock (&mon->qmp.qmp_lock);/* * FIXME @id's scope is just @mon, and broadcasting it is * wrong. If another monitor's client has a command with
@@ -4322,7 +4332,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)* etc. will be delivered to the handler side. */ g_queue_push_tail (mon->qmp.qmp_requests, req_obj); -qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_mutex_unlock (&mon->qmp.qmp_lock);/* Kick the dispatcher routine */ qemu_bh_schedule (qmp_dispatcher_bh);
Signature · 1 line
2.17.1
PATCH v6 5/13 monitor: suspend monitor instead of send CMD_DROP
In reply to Peter XuWhen we received too many qmp commands, previously we'll send COMMAND_DROPPED events to monitors, then we'll drop the requests. It can only solve the flow control of the request queue, however it'll not really work since we might queue unlimited events in the response queue which is a potential risk.
Now instead of sending such an event, we stop consuming the client input when we noticed that the queue is reaching its limitation before hand. Then after we handled commands, we'll try to resume the monitor when needed.
--- monitor.c | 46 ++++++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 16 deletions(-)
monitor.c +30 -16
@@ -165,6 +165,8 @@ struct MonFdset {QLIST_ENTRY (MonFdset) next; }; +#define QMP_REQ_QUEUE_LEN_MAX (8) + typedef struct { JSONMessageParser parser; /*
@@ -397,10 +399,21 @@ static void monitor_qmp_try_resume(Monitor *mon) {assert (monitor_is_qmp (mon));qemu_mutex_lock (&mon->qmp.qmp_lock); + +if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX ) { +/* +* This should not happen, but in case if it happens, we +* should still keep the monitor in suspend state +*/ +qemu_mutex_unlock (&mon->qmp.qmp_lock); +return ; + } +if (mon->qmp.need_resume) {monitor_resume (mon); mon->qmp.need_resume = false; } +qemu_mutex_unlock (&mon->qmp.qmp_lock); }
@@ -4254,7 +4267,14 @@ static void monitor_qmp_bh_dispatcher(void *data)qemu_bh_schedule (qmp_dispatcher_bh); } -#define QMP_REQ_QUEUE_LEN_MAX (8) +/* Called with Monitor.qmp.qmp_lock held. */ +static void monitor_qmp_suspend_locked (Monitor *mon) +{ +assert (monitor_is_qmp (mon)); +assert (mon->qmp.need_resume == false); +monitor_suspend (mon); + mon->qmp.need_resume = true; +}static void handle_qmp_command (JSONMessageParser *parser,GQueue *tokens) {
@@ -4307,22 +4327,16 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)* OOB is not enabled, the server will never drop any command. */ if (!qmp_oob_enabled (mon)) { -monitor_suspend (mon); - mon->qmp.need_resume = true; +monitor_qmp_suspend_locked (mon); }else { -/* Drop the request if queue is full. */ -if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX ) { -qemu_mutex_unlock (&mon->qmp.qmp_lock); -/* -* FIXME @id's scope is just @mon, and broadcasting it is -* wrong. If another monitor's client has a command with -* the same ID in flight, the event will incorrectly claim -* that command was dropped. -*/ -qapi_event_send_command_dropped (id, -COMMAND_DROP_REASON_QUEUE_FULL ); -qmp_request_free (req_obj); -return ; +/* +* If the queue is reaching the length limitation, we queue +* this command, meanwhile we suspend the monitor to block new +* commands. We'll resume ourselves until the queue has more +* space. +*/ +if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX -1 ) { +monitor_qmp_suspend_locked (mon); } }
Signature · 1 line
2.17.1
PATCH v6 6/13 qapi: remove COMMAND_DROPPED event
In reply to Peter XuNow it was not used any more. Drop it, especially if we can do that before we release QEMU 3.0.
--- docs/interop/qmp-spec.txt | 5 +++-- qapi/misc.json | 40 --------------------------------------- 2 files changed, 3 insertions(+), 42 deletions(-)
docs/interop/qmp-spec.txt +3 -2
@@ -131,8 +131,9 @@ to pass "id" with out-of-band commands. Passing it with all commands is recommended for clients that accept capability "oob". If the client sends in-band commands faster than the server can -execute them, the server will eventually drop commands to limit the -queue length. The sever sends event COMMAND_DROPPED then. +execute them, the server will stop reading the requests from the QMP +channel until the request queue length is reduced to an acceptable +range. Only a few commands support out-of-band execution. The ones that do have "allow-oob": true in output of query-qmp-schema.
qapi/misc.json +0 -40
@@ -3432,46 +3432,6 @@ ## { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' } -## -# @CommandDropReason: -# -# Reasons that caused one command to be dropped. -# -# @queue-full: the command queue is full. This can only occur when -# the client sends a new non-oob command before the -# response to the previous non-oob command has been -# received. -# -# Since: 2.12 -## -{ 'enum': 'CommandDropReason', - 'data': [ 'queue-full' ] } - -## -# @COMMAND_DROPPED: -# -# Emitted when a command is dropped due to some reason. Commands can -# only be dropped when the oob capability is enabled. -# -# @id: The dropped command's "id" field. -# FIXME Broken by design. Events are broadcast to all monitors. If -# another monitor's client has a command with the same ID in flight, -# the event will incorrectly claim that command was dropped. -# -# @reason: The reason why the command is dropped. -# -# Since: 2.12 -# -# Example: -# -# { "event": "COMMAND_DROPPED", -# "data": {"result": {"id": "libvirt-102", -# "reason": "queue-full" } } } -# -## -{ 'event': 'COMMAND_DROPPED' , - 'data': { 'id': 'any', 'reason': 'CommandDropReason' } } - ## # @set-numa-node: #
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event
Quoted 2 lines · Peter Xu
Now it was not used any more. Drop it, especially if we can do that before we release QEMU 3.0.
Stale commit message, now that 3.0 has been released. But still doable, since the event could not be emitted without use of the experimental x-oob option.
Quoted 6 lines
Signed-off-by Peter Xu <[email protected]>--- docs/interop/qmp-spec.txt | 5 +++-- qapi/misc.json | 40 --------------------------------------- 2 files changed, 3 insertions(+), 42 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event
Quoted 7 lines · Eric Blake
Quoted 2 lines · Peter Xu
Now it was not used any more. Drop it, especially if we can do that before we release QEMU 3.0.
Stale commit message, now that 3.0 has been released. But still doable, since the event could not be emitted without use of the experimental x-oob option.
Fixed:
Now it was not used any more, drop it. We can still do that since out-of-band is still experimental, and this event is only used when out-of-band is enabled.
Thanks,
Signature · 1 line
Peter Xu
PATCH v6 7/13 monitor: restrict response queue length too
In reply to Peter XuBefore this patch we were only monitoring the request queue, but it's still possible that a client only sends requests but it never eats any reply from us. In that case our response queue might grow with unlimited responses and put us at risk.
Now we play the similar trick as we have done to the request queue to make sure we apply the same queue length rule to the response queue as well. Then we also need to peek at the queue length after we unqueue a response now, to make sure we'll kick the monitor to alive if it was suspended due to "response queue full".
--- monitor.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
monitor.c +19 -12
@@ -394,18 +394,15 @@ static void monitor_qmp_cleanup_queues(Monitor *mon)qemu_mutex_unlock (&mon->qmp.qmp_lock); } -/* Try to resume the monitor if it was suspended due to any reason */ -static void monitor_qmp_try_resume (Monitor *mon) +/* Callers must be with Monitor.qmp.qmp_lock held. */ +static void monitor_qmp_try_resume_locked (Monitor *mon) { -assert (monitor_is_qmp (mon)); -qemu_mutex_lock (&mon->qmp.qmp_lock); - -if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX ) { +if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX || + mon->qmp.qmp_responses->length >=QMP_REQ_QUEUE_LEN_MAX ) {/* * This should not happen, but in case if it happens, we * should still keep the monitor in suspend state */ -qemu_mutex_unlock (&mon->qmp.qmp_lock);return ; }
@@ -413,7 +410,14 @@ static void monitor_qmp_try_resume(Monitor *mon)monitor_resume (mon); mon->qmp.need_resume = false; } +} +/* Try to resume the monitor if it was suspended due to any reason */ +static void monitor_qmp_try_resume (Monitor *mon) +{ +assert (monitor_is_qmp (mon)); +qemu_mutex_lock (&mon->qmp.qmp_lock); +monitor_qmp_try_resume_locked (mon);qemu_mutex_unlock (&mon->qmp.qmp_lock); }
@@ -575,6 +579,8 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)qemu_mutex_lock (&mon->qmp.qmp_lock); data =g_queue_pop_head (mon->qmp.qmp_responses); +/* In case if we were suspended due to response queue full */ +monitor_qmp_try_resume_locked (mon);qemu_mutex_unlock (&mon->qmp.qmp_lock);return data;
@@ -4330,12 +4336,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)monitor_qmp_suspend_locked (mon); }else {/* -* If the queue is reaching the length limitation, we queue -* this command, meanwhile we suspend the monitor to block new -* commands. We'll resume ourselves until the queue has more -* space. +* If any of the req/resp queue is reaching the length +* limitation, we queue this command, meanwhile we suspend the +* monitor to block new commands. We'll resume ourselves +* until both of the queues have more spaces. */ -if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX -1 ) { +if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX -1 || + mon->qmp.qmp_responses->length >=QMP_REQ_QUEUE_LEN_MAX -1 ) {monitor_qmp_suspend_locked (mon); } }
Signature · 1 line
2.17.1
PATCH v6 8/13 monitor: remove "x-oob", turn oob on by default
In reply to Peter XuOOB commands were introduced in commit cf869d53172. Unfortunately, we ran into a regression, and had to disable them by default for 2.12 (commit be933ffc23).
http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
The regression has since been fixed (commit 951702f39c7 "monitor: bind dispatch bh to iohandler context"). Time to re-enable OOB.
This patch partly reverts be933ffc23 (monitor: new parameter "x-oob"), and turns it on again for non-MUX QMPs. Note that we can't enable Out-Of-Band for monitors with MUX-typed chardev backends, because not all the chardev frontends can run without main thread, or can run in multiple threads.
Some trivial touch-up in the test code is required to make sure qmp-test won't break.
--- include/monitor/monitor.h | 1 - monitor.c | 22 ++++++---------------- tests/libqtest.c | 2 +- tests/qmp-test.c | 2 +- vl.c | 5 ----- 5 files changed, 8 insertions(+), 24 deletions(-)
include/monitor/monitor.h +0 -1
@@ -13,7 +13,6 @@ extern __thread Monitor *cur_mon;#define MONITOR_USE_READLINE 0x02#define MONITOR_USE_CONTROL 0x04#define MONITOR_USE_PRETTY 0x08 -#define MONITOR_USE_OOB 0x10bool monitor_cur_is_qmp (void );
monitor.c +6 -16
@@ -4667,19 +4667,12 @@ void monitor_init(Chardev *chr, int flags) {Monitor *mon =g_malloc (sizeof (*mon));bool use_readline = flags &MONITOR_USE_READLINE ; -bool use_oob = flags &MONITOR_USE_OOB ; - -if (use_oob) { -if (CHARDEV_IS_MUX (chr)) { -error_report ("Monitor out-of-band is not supported with " -"MUX typed chardev backend" ); -exit (1 ); - } -if (use_readline) { -error_report ("Monitor out-of-band is only supported by QMP" ); -exit (1 ); - } - } +/* +* Note: we can't enable Out-Of-Band for monitors with MUX-typed +* chardev backends, because not all the chardev frontends can run +* without main thread, or can run in multiple threads. +*/ +bool use_oob = (flags &MONITOR_USE_CONTROL ) && !CHARDEV_IS_MUX (chr);monitor_data_init (mon, false, use_oob);
@@ -4776,9 +4769,6 @@ QemuOptsList qemu_mon_opts = { },{ .name = "pretty", .type = QEMU_OPT_BOOL, - },{ - .name = "x-oob", - .type = QEMU_OPT_BOOL, }, {/* end of list */ } },
tests/libqtest.c +1 -1
@@ -213,7 +213,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,"-display none " "%s" , qemu_binary, socket_path,getenv ("QTEST_LOG" ) ?"/dev/fd/2" :"/dev/null" , - qmp_socket_path, use_oob ?",x-oob=on" :"" , + qmp_socket_path,"" , extra_args ?: "");execlp ("/bin/sh" ,"sh" ,"-c" , command,NULL );exit (1 );
tests/qmp-test.c +1 -1
@@ -89,7 +89,7 @@ static void test_qmp_protocol(void)g_assert (q);test_version (qdict_get (q,"version" )); capabilities =qdict_get_qlist (q,"capabilities" ); -g_assert (capabilities &&qlist_empty (capabilities)); +g_assert (capabilities);qobject_unref (resp);/* Test valid command before handshake */
vl.c +0 -5
@@ -2323,11 +2323,6 @@ static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)if (qemu_opt_get_bool (opts,"pretty" ,0 )) flags |=MONITOR_USE_PRETTY ; -/* OOB is off by default */ -if (qemu_opt_get_bool (opts,"x-oob" ,0 )) { - flags |=MONITOR_USE_OOB ; - } - chardev =qemu_opt_get (opts,"chardev" ); chr =qemu_chr_find (chardev);if (chr ==NULL ) {
Signature · 1 line
2.17.1
PATCH v6 9/13 Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
In reply to Peter XuThis reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a.
Meanwhile, revert one line from fa198ad9bdef to make sure qtest_init_without_qmp_handshake() will only pass in one parameter.
--- tests/libqtest.c | 10 ++++------ tests/libqtest.h | 4 +--- tests/qmp-test.c | 4 ++-- 3 files changed, 7 insertions(+), 11 deletions(-)
tests/libqtest.c +4 -6
@@ -173,8 +173,7 @@ static const char *qtest_qemu_binary(void)return qemu_bin; } -QTestState *qtest_init_without_qmp_handshake (bool use_oob, -const char *extra_args) +QTestState *qtest_init_without_qmp_handshake (const char *extra_args) {QTestState *s;int sock, qmpsock, i;
@@ -207,13 +206,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, command =g_strdup_printf ("exec %s " "-qtest unix:%s,nowait " "-qtest-log %s " -"-chardev socket,path=%s,nowait,id=char0 " -"-mon chardev=char0,mode=control%s " +"-qmp unix:%s,nowait " "-machine accel=qtest " "-display none " "%s" , qemu_binary, socket_path,getenv ("QTEST_LOG" ) ?"/dev/fd/2" :"/dev/null" , - qmp_socket_path,"" , + qmp_socket_path, extra_args ?:"" );execlp ("/bin/sh" ,"sh" ,"-c" , command,NULL );exit (1 );
@@ -248,7 +246,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob,QTestState *qtest_init (const char *extra_args) { -QTestState *s =qtest_init_without_qmp_handshake (false, extra_args); +QTestState *s =qtest_init_without_qmp_handshake (extra_args);/* Read the QMP greeting and then do the handshake */ qtest_qmp_discard_response (s,"" );
tests/libqtest.h +1 -3
@@ -56,14 +56,12 @@ QTestState *qtest_init(const char *extra_args);/** * qtest_init_without_qmp_handshake: -* @use_oob: true to have the server advertise OOB support * @extra_args: other arguments to pass to QEMU. CAUTION: these * arguments are subject to word splitting and shell evaluation. * * Returns: #QTestState instance. */ -QTestState *qtest_init_without_qmp_handshake (bool use_oob, -const char *extra_args); +QTestState *qtest_init_without_qmp_handshake (const char *extra_args);/** * qtest_quit:
tests/qmp-test.c +2 -2
@@ -81,7 +81,7 @@ static void test_qmp_protocol(void)QList *capabilities;QTestState *qts; - qts =qtest_init_without_qmp_handshake (false, common_args); + qts =qtest_init_without_qmp_handshake (common_args);/* Test greeting */ resp =qtest_qmp_receive (qts);
@@ -195,7 +195,7 @@ static void test_qmp_oob(void)QList *capabilities;QString *qstr; - qts =qtest_init_without_qmp_handshake (true, common_args); + qts =qtest_init_without_qmp_handshake (common_args);/* Check the greeting message. */ resp =qtest_qmp_receive (qts);
Signature · 1 line
2.17.1
PATCH v6 10/13 monitor: add traces for qmp queues
In reply to Peter XuThis can help us to track QMP monitor flow controls.
--- monitor.c | 8 ++++++++ trace-events | 4 ++++ 2 files changed, 12 insertions(+)
monitor.c +8 -0
@@ -376,6 +376,7 @@ static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)while (!g_queue_is_empty (mon->qmp.qmp_requests)) {qmp_request_free (g_queue_pop_head (mon->qmp.qmp_requests)); } +trace_monitor_qmp_request_queue (mon,0 ); }/* Caller must hold the mon->qmp.qmp_lock */
@@ -384,6 +385,7 @@ static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)while (!g_queue_is_empty (mon->qmp.qmp_responses)) {qobject_unref ((QDict *)g_queue_pop_head (mon->qmp.qmp_responses)); } +trace_monitor_qmp_response_queue (mon,0 ); }static void monitor_qmp_cleanup_queues (Monitor *mon)
@@ -409,6 +411,7 @@ static void monitor_qmp_try_resume_locked(Monitor *mon)if (mon->qmp.need_resume) {monitor_resume (mon); mon->qmp.need_resume = false; +trace_monitor_qmp_resume (mon); } }
@@ -556,6 +559,7 @@ static void qmp_queue_response(Monitor *mon, QDict *rsp)*/ qemu_mutex_lock (&mon->qmp.qmp_lock);g_queue_push_tail (mon->qmp.qmp_responses,qobject_ref (rsp)); +trace_monitor_qmp_response_queue (mon, mon->qmp.qmp_responses->length);qemu_mutex_unlock (&mon->qmp.qmp_lock);qemu_bh_schedule (qmp_respond_bh); }else {
@@ -579,6 +583,7 @@ static QDict *monitor_qmp_response_pop_one(Monitor *mon)qemu_mutex_lock (&mon->qmp.qmp_lock); data =g_queue_pop_head (mon->qmp.qmp_responses); +trace_monitor_qmp_response_queue (mon, mon->qmp.qmp_responses->length);/* In case if we were suspended due to response queue full */ monitor_qmp_try_resume_locked (mon);qemu_mutex_unlock (&mon->qmp.qmp_lock);
@@ -4223,6 +4228,7 @@ static QMPRequest *monitor_qmp_requests_pop_any(void) QTAILQ_FOREACH(mon, &mon_list, entry) {qemu_mutex_lock (&mon->qmp.qmp_lock); req_obj =g_queue_pop_head (mon->qmp.qmp_requests); +trace_monitor_qmp_request_queue (mon, mon->qmp.qmp_requests->length);qemu_mutex_unlock (&mon->qmp.qmp_lock);if (req_obj) {break ;
@@ -4280,6 +4286,7 @@ static void monitor_qmp_suspend_locked(Monitor *mon)assert (mon->qmp.need_resume == false);monitor_suspend (mon); mon->qmp.need_resume = true; +trace_monitor_qmp_suspend (mon); }static void handle_qmp_command (JSONMessageParser *parser,GQueue *tokens)
@@ -4353,6 +4360,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)* etc. will be delivered to the handler side. */ g_queue_push_tail (mon->qmp.qmp_requests, req_obj); +trace_monitor_qmp_request_queue (mon, mon->qmp.qmp_requests->length);qemu_mutex_unlock (&mon->qmp.qmp_lock);/* Kick the dispatcher routine */
trace-events +4 -0
@@ -50,6 +50,10 @@ handle_qmp_command(void *mon, const char *req) "mon %p req: %s" monitor_suspend(void *ptr, int cnt) "mon %p: %d" monitor_qmp_cmd_in_band(const char *id) "%s" monitor_qmp_cmd_out_of_band(const char *id) "%s" +monitor_qmp_suspend(void *mon) "mon=%p" +monitor_qmp_resume(void *mon) "mon=%p" +monitor_qmp_request_queue(void *mon, int len) "mon=%p len=%d" +monitor_qmp_response_queue(void *mon, int len) "mon=%p len=%d" # dma-helpers.c dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
Signature · 1 line
2.17.1
PATCH v6 11/13 tests: remove "0.15" prefix for test-qmp-cmds
In reply to Peter XuThe "0.15" prefix seems useless. Remove them.
--- tests/test-qmp-cmds.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
tests/test-qmp-cmds.c +5 -5
@@ -286,11 +286,11 @@ int main(int argc, char **argv) {g_test_init (&argc, &argv,NULL ); -g_test_add_func ("/0.15/dispatch_cmd" , test_dispatch_cmd); -g_test_add_func ("/0.15/dispatch_cmd_failure" , test_dispatch_cmd_failure); -g_test_add_func ("/0.15/dispatch_cmd_io" , test_dispatch_cmd_io); -g_test_add_func ("/0.15/dealloc_types" , test_dealloc_types); -g_test_add_func ("/0.15/dealloc_partial" , test_dealloc_partial); +g_test_add_func ("/dispatch_cmd" , test_dispatch_cmd); +g_test_add_func ("/dispatch_cmd_failure" , test_dispatch_cmd_failure); +g_test_add_func ("/dispatch_cmd_io" , test_dispatch_cmd_io); +g_test_add_func ("/dealloc_types" , test_dealloc_types); +g_test_add_func ("/dealloc_partial" , test_dealloc_partial);test_qmp_init_marshal (&qmp_commands);g_test_run ();
Signature · 1 line
2.17.1
Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
Quoted 15 lines · Peter Xu
The "0.15" prefix seems useless. Remove them.
Signed-off-by Peter Xu <[email protected]>--- tests/test-qmp-cmds.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)tests/test-qmp-cmds.c +0
-0@@ -286,11 +286,11 @@ int main(int argc, char **argv) {g_test_init (&argc, &argv,NULL );
tests/test-qmp-cmds.c int main(int argc, char **argv) · line 293
Quoted 10 lines
tests/test-qmp-cmds.c +5
-5-g_test_add_func ("/0.15/dispatch_cmd" , test_dispatch_cmd); -g_test_add_func ("/0.15/dispatch_cmd_failure" , test_dispatch_cmd_failure); -g_test_add_func ("/0.15/dispatch_cmd_io" , test_dispatch_cmd_io); -g_test_add_func ("/0.15/dealloc_types" , test_dealloc_types); -g_test_add_func ("/0.15/dealloc_partial" , test_dealloc_partial); +g_test_add_func ("/dispatch_cmd" , test_dispatch_cmd); +g_test_add_func ("/dispatch_cmd_failure" , test_dispatch_cmd_failure); +g_test_add_func ("/dispatch_cmd_io" , test_dispatch_cmd_io); +g_test_add_func ("/dealloc_types" , test_dealloc_types); +g_test_add_func ("/dealloc_partial" , test_dealloc_partial);
Most other tests use a common prefix, so maybe rather replace the "0.15" with "qmp-cmd" here? E.g.:
g_test_add_func("/qmp-cmd/dispatch_cmd", test_dispatch_cmd);
?
Thomas
Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
Quoted 30 lines · Thomas Huth
Quoted 15 lines · Peter Xu
The "0.15" prefix seems useless. Remove them.
Signed-off-by Peter Xu <[email protected]>--- tests/test-qmp-cmds.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)tests/test-qmp-cmds.c +0
-0@@ -286,11 +286,11 @@ int main(int argc, char **argv) {g_test_init (&argc, &argv,NULL );tests/test-qmp-cmds.c int main(int argc, char **argv) · line 293
Quoted 10 lines
tests/test-qmp-cmds.c +5
-5-g_test_add_func ("/0.15/dispatch_cmd" , test_dispatch_cmd); -g_test_add_func ("/0.15/dispatch_cmd_failure" , test_dispatch_cmd_failure); -g_test_add_func ("/0.15/dispatch_cmd_io" , test_dispatch_cmd_io); -g_test_add_func ("/0.15/dealloc_types" , test_dealloc_types); -g_test_add_func ("/0.15/dealloc_partial" , test_dealloc_partial); +g_test_add_func ("/dispatch_cmd" , test_dispatch_cmd); +g_test_add_func ("/dispatch_cmd_failure" , test_dispatch_cmd_failure); +g_test_add_func ("/dispatch_cmd_io" , test_dispatch_cmd_io); +g_test_add_func ("/dealloc_types" , test_dealloc_types); +g_test_add_func ("/dealloc_partial" , test_dealloc_partial);Most other tests use a common prefix, so maybe rather replace the "0.15" with "qmp-cmd" here? E.g.:
Quoted 3 lines
g_test_add_func("/qmp-cmd/dispatch_cmd", test_dispatch_cmd);?
I'll drop this patch due to a dup in master. That one has "/qmp" prefix. Thanks Thomas.
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
In reply to Peter XuQuoted 3 lines · Peter Xu
The "0.15" prefix seems useless. Remove them.
Signed-off-by Peter Xu <[email protected]>
I sent a similar patch long ago, and it is finally queued by Markus! :) "tests: change /0.15/* tests to /qmp/*"
Quoted 28 lines
--- tests/test-qmp-cmds.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)tests/test-qmp-cmds.c +5
-6@@ -286,11 +286,11 @@ int main(int argc, char **argv) {g_test_init (&argc, &argv,NULL ); -g_test_add_func ("/0.15/dispatch_cmd" , test_dispatch_cmd); -g_test_add_func ("/0.15/dispatch_cmd_failure" , test_dispatch_cmd_failure); -g_test_add_func ("/0.15/dispatch_cmd_io" , test_dispatch_cmd_io); -g_test_add_func ("/0.15/dealloc_types" , test_dealloc_types); -g_test_add_func ("/0.15/dealloc_partial" , test_dealloc_partial); +g_test_add_func ("/dispatch_cmd" , test_dispatch_cmd); +g_test_add_func ("/dispatch_cmd_failure" , test_dispatch_cmd_failure); +g_test_add_func ("/dispatch_cmd_io" , test_dispatch_cmd_io); +g_test_add_func ("/dealloc_types" , test_dealloc_types); +g_test_add_func ("/dealloc_partial" , test_dealloc_partial);test_qmp_init_marshal (&qmp_commands);g_test_run (); --2.17.1
Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
Quoted 7 lines · Marc-André Lureau
Quoted 3 lines · Peter Xu
The "0.15" prefix seems useless. Remove them.
Signed-off-by Peter Xu <[email protected]>I sent a similar patch long ago, and it is finally queued by Markus! :) "tests: change /0.15/* tests to /qmp/*"
Oh! I guess I rebased to a not-the-latest master tree due to some unknown reason...
Thanks for mentioning. I'll drop this patch in my next post (and I'll also modify the next patch to match the names).
Regards,
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
Quoted 11 lines · Peter Xu
Quoted 7 lines · Marc-André Lureau
Quoted 3 lines · Peter Xu
The "0.15" prefix seems useless. Remove them.
Signed-off-by Peter Xu <[email protected]>I sent a similar patch long ago, and it is finally queued by Markus! :) "tests: change /0.15/* tests to /qmp/*"
Oh! I guess I rebased to a not-the-latest master tree due to some unknown reason...
I think you simply sent your patch before my pull request went through.
Quoted 2 lines
Thanks for mentioning. I'll drop this patch in my next post (and I'll also modify the next patch to match the names).
Thanks!
PATCH v6 12/13 tests: add oob functional test for test-qmp-cmds
In reply to Peter XuStraightforward test just to let the test-qmp-cmds be complete.
--- tests/test-qmp-cmds.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
tests/test-qmp-cmds.c +16 -0
@@ -122,6 +122,21 @@ static void test_dispatch_cmd(void)qobject_unref (req); } +static void test_dispatch_cmd_oob (void ) +{ +QDict *req =qdict_new (); +QDict *resp; + +qdict_put_str (req,"exec-oob" ,"test-flags-command" ); + + resp =qmp_dispatch (&qmp_commands,QOBJECT (req), true); +assert (resp !=NULL ); +assert (!qdict_haskey (resp,"error" )); + +qobject_unref (resp); +qobject_unref (req); +} +/* test commands that return an error due to invalid parameters */ static void test_dispatch_cmd_failure (void ) {
@@ -287,6 +302,7 @@ int main(int argc, char **argv)g_test_init (&argc, &argv,NULL );g_test_add_func ("/dispatch_cmd" , test_dispatch_cmd); +g_test_add_func ("/dispatch_cmd_oob" , test_dispatch_cmd_oob);g_test_add_func ("/dispatch_cmd_failure" , test_dispatch_cmd_failure);g_test_add_func ("/dispatch_cmd_io" , test_dispatch_cmd_io);g_test_add_func ("/dealloc_types" , test_dealloc_types);
Signature · 1 line
2.17.1
PATCH v6 13/13 monitor: reduce different code path for oob
In reply to Peter XuMarkus suggests that we should reduce special code paths for out-of-band if possible, so that QMP logic can be simplified. Apply this rule to two places where use_io_thread is used but not completely necessary.
--- monitor.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-)
monitor.c +11 -22
@@ -552,23 +552,15 @@ static void qmp_send_response(Monitor *mon, QDict *rsp)static void qmp_queue_response (Monitor *mon,QDict *rsp) { -if (mon->use_io_thread) { -/* -* Push a reference to the response queue. The I/O thread -* drains that queue and emits. -*/ -qemu_mutex_lock (&mon->qmp.qmp_lock); -g_queue_push_tail (mon->qmp.qmp_responses,qobject_ref (rsp)); -trace_monitor_qmp_response_queue (mon, mon->qmp.qmp_responses->length); -qemu_mutex_unlock (&mon->qmp.qmp_lock); -qemu_bh_schedule (qmp_respond_bh); - }else { -/* -* Not using monitor I/O thread, i.e. we are in the main thread. -* Emit right away. -*/ -qmp_send_response (mon, rsp); - } +/* +* Push a reference to the response queue. The I/O thread drains +* that queue and emits. +*/ +qemu_mutex_lock (&mon->qmp.qmp_lock); +g_queue_push_tail (mon->qmp.qmp_responses,qobject_ref (rsp)); +trace_monitor_qmp_response_queue (mon, mon->qmp.qmp_responses->length); +qemu_mutex_unlock (&mon->qmp.qmp_lock); +qemu_bh_schedule (qmp_respond_bh); }struct QMPResponse {
@@ -4433,12 +4425,9 @@ void monitor_resume(Monitor *mon)if (atomic_dec_fetch (&mon->suspend_cnt) ==0 ) {if (monitor_is_qmp (mon)) {/* -* For QMP monitors that are running in the I/O thread, -* let's kick the thread in case it's sleeping. +* Let's kick the thread in case it's sleeping. */ -if (mon->use_io_thread) { -aio_notify (iothread_get_aio_context (mon_iothread)); - } +aio_notify (iothread_get_aio_context (mon_iothread)); }else {assert (mon->rs);readline_show_prompt (mon->rs);
Signature · 1 line
2.17.1
Quoted 21 lines · Peter Xu
Hi, Markus, Marc-Andre,
I didn't follow up the latest discussions on the response queue or monitor changes. For now this series still makes sense to me, so I'm just rebasing the series to master and repost (with some additional changes/patches to address Markus's concerns). Please let me know if you have made any conclusion that I'm not aware of, or if this work needs any further rebasing.
v6 (previous version of the series might be incorrect; from this version, I continued to use the old version counts):
- collect r-bs
- use Markus's version ofbuild_params()[Markus]
- use \' instead of \" in Python codes [Markus]
- in qapi_event_send_*() still pass in&error_abortinstead of NULL [Markus]
- removeCOMMAND_DROPPEDfrom document too
- add missing pieces for test-qmp-cmds [Markus]
- add one patch to remove as much different code path for oob as possible [Markus]
Quoted 6 lines
TODO:
- flow control test is still missing; I still didn't consider it much, also we possibly need to touch upmonitor_puts()to use limited sized buffer to complete the flow control of QMP channelPlease review. Thanks,
PATCH 01-03 queued. Thanks!