* [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-21 18:13 ` Marc-André Lureau
2018-08-27 11:29 ` Markus Armbruster
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 02/13] qapi: Fix build_params() for empty parameter list Peter Xu
` (12 subsequent siblings)
13 siblings, 2 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
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 <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/monitor.c b/monitor.c
index 77861e96af..5cd9398824 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh Peter Xu
@ 2018-08-21 18:13 ` Marc-André Lureau
2018-08-22 4:38 ` Peter Xu
2018-08-27 11:29 ` Markus Armbruster
1 sibling, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-08-21 18:13 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Daniel P . Berrange, Markus Armbruster,
Dr . David Alan Gilbert
Hi
On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote:
> 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 <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
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
> ---
> monitor.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 77861e96af..5cd9398824 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
2018-08-21 18:13 ` Marc-André Lureau
@ 2018-08-22 4:38 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-22 4:38 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eric Blake, Daniel P . Berrange, Markus Armbruster,
Dr . David Alan Gilbert
On Tue, Aug 21, 2018 at 08:13:59PM +0200, Marc-André Lureau wrote:
> Hi
>
> On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote:
> > 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 <armbru@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
>
> 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,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh Peter Xu
2018-08-21 18:13 ` Marc-André Lureau
@ 2018-08-27 11:29 ` Markus Armbruster
2018-08-28 3:26 ` Peter Xu
1 sibling, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2018-08-27 11:29 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
Peter Xu <peterx@redhat.com> writes:
> 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 <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> monitor.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 77861e96af..5cd9398824 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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 */
Mind if I drop this comment?
> + 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.
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh
2018-08-27 11:29 ` Markus Armbruster
@ 2018-08-28 3:26 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-28 3:26 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
On Mon, Aug 27, 2018 at 01:29:29PM +0200, Markus Armbruster wrote:
> > @@ -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 */
>
> Mind if I drop this comment?
Yes, please.
>
> > + 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,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v6 02/13] qapi: Fix build_params() for empty parameter list
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission Peter Xu
` (11 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
From: Markus Armbruster <armbru@redhat.com>
build_params() returns '' instead of 'void' when there are no
parameters. Can't happen now, but the next commit will change that.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
[peterx: compose the patch from email replies]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
scripts/qapi/common.py | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 9230a2a3e8..6471c45233 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -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'
#
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 01/13] monitor: simplify monitor_qmp_setup_handlers_bh Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 02/13] qapi: Fix build_params() for empty parameter list Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-27 12:14 ` Markus Armbruster
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 04/13] monitor: move need_resume flag into monitor struct Peter Xu
` (10 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
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.
Then, remove that parameter from all the callers to send an event.
Suggested-by: Eric Blake <eblake@redhat.com>
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index f2f75a977d..33de5fa23d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -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);
}
diff --git a/block/qcow2.c b/block/qcow2.c
index ec9e6238a0..c13153735a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -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) {
diff --git a/block/quorum.c b/block/quorum.c
index 9152da8c58..eb526cc0f1 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -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);
diff --git a/block/write-threshold.c b/block/write-threshold.c
index 1d48fc2077..85b78dc2a9 100644
--- a/block/write-threshold.c
+++ b/block/write-threshold.c
@@ -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);
diff --git a/blockjob.c b/blockjob.c
index be5903aa96..bf7ef48f98 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -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);
diff --git a/cpus.c b/cpus.c
index b5844b7103..488a2b5c62 100644
--- a/cpus.c
+++ b/cpus.c
@@ -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();
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index c2e11465f0..6d3cffd548 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -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);
}
diff --git a/dump.c b/dump.c
index 04467b353e..2d8f6b3231 100644
--- a/dump.c
+++ b/dump.c
@@ -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);
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index b8d39012cd..aafdc61648 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -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;
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 5ae595ecbe..d19b7722f0 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -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);
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 0ff1712c4c..8c7c1013f3 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -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;
}
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 679a824888..2cbe1d4ed5 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -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);
}
}
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 529b82de18..36b788a66b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -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;
}
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index f154756e85..4bdd5b8532 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -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 */
diff --git a/hw/ppc/spapr_rtc.c b/hw/ppc/spapr_rtc.c
index a37360537e..cd049f389d 100644
--- a/hw/ppc/spapr_rtc.c
+++ b/hw/ppc/spapr_rtc.c
@@ -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);
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 6f1f723b1f..b941aa1966 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -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)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 1f7a87f094..fcd41d314e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -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);
}
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 6e8ba061d8..33e6c20184 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -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;
diff --git a/include/qapi/qmp-event.h b/include/qapi/qmp-event.h
index 0c87ad833e..23e588ccf8 100644
--- a/include/qapi/qmp-event.h
+++ b/include/qapi/qmp-event.h
@@ -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);
diff --git a/job.c b/job.c
index fa671b431a..03dfc59bcd 100644
--- a/job.c
+++ b/job.c
@@ -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);
}
}
diff --git a/migration/migration.c b/migration/migration.c
index b7d9854bda..292ffe2c3a 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -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)) {
diff --git a/migration/ram.c b/migration/ram.c
index 24dea2730c..fa02d3d358 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -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);
}
}
diff --git a/monitor.c b/monitor.c
index 5cd9398824..3fb480d94b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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;
}
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 764ef177ab..2ed7902424 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -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);
}
''')
diff --git a/scsi/pr-manager-helper.c b/scsi/pr-manager-helper.c
index 3027dde60d..438380fced 100644
--- a/scsi/pr-manager-helper.c
+++ b/scsi/pr-manager-helper.c
@@ -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);
}
}
diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
index 8677094ad1..9cddd72adb 100644
--- a/tests/test-qmp-event.c
+++ b/tests/test-qmp-event.c
@@ -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);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index f8c0878529..a4fbbc3898 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -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;
}
diff --git a/ui/vnc.c b/ui/vnc.c
index 359693238b..2345bd054a 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -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;
diff --git a/vl.c b/vl.c
index 16b913f9d5..a76d3ab729 100644
--- a/vl.c
+++ b/vl.c
@@ -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();
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission Peter Xu
@ 2018-08-27 12:14 ` Markus Armbruster
2018-08-28 3:31 ` Peter Xu
2018-08-30 14:05 ` Markus Armbruster
0 siblings, 2 replies; 29+ messages in thread
From: Markus Armbruster @ 2018-08-27 12:14 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
Peter Xu <peterx@redhat.com> writes:
> 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.
> 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".
> Suggested-by: Eric Blake <eblake@redhat.com>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
Patch quoted except for the parts that drop &error_abort from
qapi_event_send_FOO() calls:
[...]
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index c2e11465f0..6d3cffd548 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -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);
> }
>
[...]
> diff --git a/include/qapi/qmp-event.h b/include/qapi/qmp-event.h
> index 0c87ad833e..23e588ccf8 100644
> --- a/include/qapi/qmp-event.h
> +++ b/include/qapi/qmp-event.h
> @@ -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);
>
[...]
> diff --git a/migration/ram.c b/migration/ram.c
> index 79c89425a3..f6fd8e5e09 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -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.
> diff --git a/monitor.c b/monitor.c
> index 5cd9398824..3fb480d94b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -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.
[...]
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index 764ef177ab..2ed7902424 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -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".
> %(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);
> ''')
> 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.
> ''',
> 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);
Likewise.
> 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);
@emit is either monitor_qapi_event_queue() or event_test_emit().
Neither can fail.
>
> ''',
> 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);
> }
> ''')
[...]
> diff --git a/tests/test-qmp-event.c b/tests/test-qmp-event.c
> index 8677094ad1..9cddd72adb 100644
> --- a/tests/test-qmp-event.c
> +++ b/tests/test-qmp-event.c
> @@ -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:
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
2018-08-27 12:14 ` Markus Armbruster
@ 2018-08-28 3:31 ` Peter Xu
2018-08-30 14:05 ` Markus Armbruster
1 sibling, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-28 3:31 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, Dr . David Alan Gilbert, Marc-André Lureau
On Mon, Aug 27, 2018 at 02:14:25PM +0200, Markus Armbruster wrote:
[...]
> 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.
I'm stealing this into my local tree.
>
> With something like that:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks for the analysis and r-b; stealing the r-b too.
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission
2018-08-27 12:14 ` Markus Armbruster
2018-08-28 3:31 ` Peter Xu
@ 2018-08-30 14:05 ` Markus Armbruster
1 sibling, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2018-08-30 14:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Xu, qemu-devel, Dr . David Alan Gilbert,
Marc-André Lureau
Markus Armbruster <armbru@redhat.com> writes:
> Peter Xu <peterx@redhat.com> writes:
>
>> 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.
>
>> 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".
>
>> Suggested-by: Eric Blake <eblake@redhat.com>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>
> Patch quoted except for the parts that drop &error_abort from
> qapi_event_send_FOO() calls:
>
> [...]
>> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> index c2e11465f0..6d3cffd548 100644
>> --- a/docs/devel/qapi-code-gen.txt
>> +++ b/docs/devel/qapi-code-gen.txt
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,
>> @@ -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);
>> }
>>
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v6 04/13] monitor: move need_resume flag into monitor struct
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (2 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 03/13] qapi: remove error checks for event emission Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 05/13] monitor: suspend monitor instead of send CMD_DROP Peter Xu
` (9 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
It 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.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 72 +++++++++++++++++++++++++++++++------------------------
1 file changed, 41 insertions(+), 31 deletions(-)
diff --git a/monitor.c b/monitor.c
index 3fb480d94b..d31de95141 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 05/13] monitor: suspend monitor instead of send CMD_DROP
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (3 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 04/13] monitor: move need_resume flag into monitor struct Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event Peter Xu
` (8 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
When 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.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 46 ++++++++++++++++++++++++++++++----------------
1 file changed, 30 insertions(+), 16 deletions(-)
diff --git a/monitor.c b/monitor.c
index d31de95141..2fc480d75b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (4 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 05/13] monitor: suspend monitor instead of send CMD_DROP Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-27 19:30 ` Eric Blake
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 07/13] monitor: restrict response queue length too Peter Xu
` (7 subsequent siblings)
13 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
Now it was not used any more. Drop it, especially if we can do that
before we release QEMU 3.0.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
docs/interop/qmp-spec.txt | 5 +++--
qapi/misc.json | 40 ---------------------------------------
2 files changed, 3 insertions(+), 42 deletions(-)
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index 1566b8ae5e..4e24a149e2 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -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.
diff --git a/qapi/misc.json b/qapi/misc.json
index d450cfef21..2c1a6119bf 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -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:
#
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event Peter Xu
@ 2018-08-27 19:30 ` Eric Blake
2018-08-28 3:38 ` Peter Xu
0 siblings, 1 reply; 29+ messages in thread
From: Eric Blake @ 2018-08-27 19:30 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Marc-André Lureau, Daniel P . Berrange, Markus Armbruster,
Dr . David Alan Gilbert
On 08/15/2018 08:37 AM, Peter Xu wrote:
> 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.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> docs/interop/qmp-spec.txt | 5 +++--
> qapi/misc.json | 40 ---------------------------------------
> 2 files changed, 3 insertions(+), 42 deletions(-)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event
2018-08-27 19:30 ` Eric Blake
@ 2018-08-28 3:38 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-28 3:38 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert
On Mon, Aug 27, 2018 at 02:30:12PM -0500, Eric Blake wrote:
> On 08/15/2018 08:37 AM, Peter Xu wrote:
> > 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,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v6 07/13] monitor: restrict response queue length too
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (5 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 06/13] qapi: remove COMMAND_DROPPED event Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 08/13] monitor: remove "x-oob", turn oob on by default Peter Xu
` (6 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
Before 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".
Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/monitor.c b/monitor.c
index 2fc480d75b..27e8dd85be 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 08/13] monitor: remove "x-oob", turn oob on by default
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (6 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 07/13] monitor: restrict response queue length too Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 09/13] Revert "tests: Add parameter to qtest_init_without_qmp_handshake" Peter Xu
` (5 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
OOB 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.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
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(-)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 2ef5e04b37..00ded7972c 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -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 0x10
bool monitor_cur_is_qmp(void);
diff --git a/monitor.c b/monitor.c
index 27e8dd85be..10ed853de9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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 */ }
},
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 098af6aec4..c5cb3f925c 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -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);
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index b9774084f8..8d89f02d2e 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -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 */
diff --git a/vl.c b/vl.c
index a76d3ab729..64fb83df04 100644
--- a/vl.c
+++ b/vl.c
@@ -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) {
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 09/13] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (7 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 08/13] monitor: remove "x-oob", turn oob on by default Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 10/13] monitor: add traces for qmp queues Peter Xu
` (4 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a.
Meanwhile, revert one line from fa198ad9bdef to make sure
qtest_init_without_qmp_handshake() will only pass in one parameter.
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/libqtest.c | 10 ++++------
tests/libqtest.h | 4 +---
tests/qmp-test.c | 4 ++--
3 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/tests/libqtest.c b/tests/libqtest.c
index c5cb3f925c..51af4a289e 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -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, "");
diff --git a/tests/libqtest.h b/tests/libqtest.h
index ac52872cbe..180d2cc6ff 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -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:
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 8d89f02d2e..f4ac141425 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -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);
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 10/13] monitor: add traces for qmp queues
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (8 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 09/13] Revert "tests: Add parameter to qtest_init_without_qmp_handshake" Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds Peter Xu
` (3 subsequent siblings)
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
This can help us to track QMP monitor flow controls.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 8 ++++++++
trace-events | 4 ++++
2 files changed, 12 insertions(+)
diff --git a/monitor.c b/monitor.c
index 10ed853de9..b24a934437 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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 */
diff --git a/trace-events b/trace-events
index c445f54773..bd9dade938 100644
--- a/trace-events
+++ b/trace-events
@@ -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"
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (9 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 10/13] monitor: add traces for qmp queues Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:48 ` Thomas Huth
2018-08-15 18:11 ` Marc-André Lureau
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 12/13] tests: add oob functional test " Peter Xu
` (2 subsequent siblings)
13 siblings, 2 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
The "0.15" prefix seems useless. Remove them.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/test-qmp-cmds.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ba41a6161e..989819b26d 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -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
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds Peter Xu
@ 2018-08-15 13:48 ` Thomas Huth
2018-08-16 1:55 ` Peter Xu
2018-08-15 18:11 ` Marc-André Lureau
1 sibling, 1 reply; 29+ messages in thread
From: Thomas Huth @ 2018-08-15 13:48 UTC (permalink / raw)
To: Peter Xu, qemu-devel
Cc: Dr . David Alan Gilbert, Markus Armbruster,
Marc-André Lureau
On 08/15/2018 03:37 PM, Peter Xu wrote:
> The "0.15" prefix seems useless. Remove them.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> tests/test-qmp-cmds.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index ba41a6161e..989819b26d 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -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);
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
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
2018-08-15 13:48 ` Thomas Huth
@ 2018-08-16 1:55 ` Peter Xu
0 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-16 1:55 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Dr . David Alan Gilbert, Markus Armbruster,
Marc-André Lureau
On Wed, Aug 15, 2018 at 03:48:57PM +0200, Thomas Huth wrote:
> On 08/15/2018 03:37 PM, Peter Xu wrote:
> > The "0.15" prefix seems useless. Remove them.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > tests/test-qmp-cmds.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> > index ba41a6161e..989819b26d 100644
> > --- a/tests/test-qmp-cmds.c
> > +++ b/tests/test-qmp-cmds.c
> > @@ -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);
>
> 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);
>
> ?
I'll drop this patch due to a dup in master. That one has "/qmp"
prefix. Thanks Thomas.
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds Peter Xu
2018-08-15 13:48 ` Thomas Huth
@ 2018-08-15 18:11 ` Marc-André Lureau
2018-08-16 1:51 ` Peter Xu
1 sibling, 1 reply; 29+ messages in thread
From: Marc-André Lureau @ 2018-08-15 18:11 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Eric Blake, Daniel P . Berrange, Markus Armbruster,
Dr . David Alan Gilbert
On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote:
> The "0.15" prefix seems useless. Remove them.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
I sent a similar patch long ago, and it is finally queued by Markus!
:) "tests: change /0.15/* tests to /qmp/*"
> ---
> tests/test-qmp-cmds.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index ba41a6161e..989819b26d 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -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
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
2018-08-15 18:11 ` Marc-André Lureau
@ 2018-08-16 1:51 ` Peter Xu
2018-08-16 7:20 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: Peter Xu @ 2018-08-16 1:51 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, Eric Blake, Daniel P . Berrange, Markus Armbruster,
Dr . David Alan Gilbert
On Wed, Aug 15, 2018 at 08:11:22PM +0200, Marc-André Lureau wrote:
> On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote:
> > The "0.15" prefix seems useless. Remove them.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
>
> 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,
--
Peter Xu
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds
2018-08-16 1:51 ` Peter Xu
@ 2018-08-16 7:20 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2018-08-16 7:20 UTC (permalink / raw)
To: Peter Xu; +Cc: Marc-André Lureau, qemu-devel, Dr . David Alan Gilbert
Peter Xu <peterx@redhat.com> writes:
> On Wed, Aug 15, 2018 at 08:11:22PM +0200, Marc-André Lureau wrote:
>> On Wed, Aug 15, 2018 at 3:37 PM, Peter Xu <peterx@redhat.com> wrote:
>> > The "0.15" prefix seems useless. Remove them.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>>
>> 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.
> 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!
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v6 12/13] tests: add oob functional test for test-qmp-cmds
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (10 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 11/13] tests: remove "0.15" prefix for test-qmp-cmds Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 13/13] monitor: reduce different code path for oob Peter Xu
2018-08-28 19:05 ` [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Markus Armbruster
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
Straightforward test just to let the test-qmp-cmds be complete.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tests/test-qmp-cmds.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 989819b26d..a51432f09d 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -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);
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [Qemu-devel] [PATCH v6 13/13] monitor: reduce different code path for oob
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (11 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 12/13] tests: add oob functional test " Peter Xu
@ 2018-08-15 13:37 ` Peter Xu
2018-08-28 19:05 ` [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Markus Armbruster
13 siblings, 0 replies; 29+ messages in thread
From: Peter Xu @ 2018-08-15 13:37 UTC (permalink / raw)
To: qemu-devel
Cc: Eric Blake, Marc-André Lureau, Daniel P . Berrange,
Markus Armbruster, Dr . David Alan Gilbert, peterx
Markus 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.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 33 +++++++++++----------------------
1 file changed, 11 insertions(+), 22 deletions(-)
diff --git a/monitor.c b/monitor.c
index b24a934437..7494885890 100644
--- a/monitor.c
+++ b/monitor.c
@@ -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);
--
2.17.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default
2018-08-15 13:37 [Qemu-devel] [PATCH v6 00/13] monitor: enable OOB by default Peter Xu
` (12 preceding siblings ...)
2018-08-15 13:37 ` [Qemu-devel] [PATCH v6 13/13] monitor: reduce different code path for oob Peter Xu
@ 2018-08-28 19:05 ` Markus Armbruster
13 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2018-08-28 19:05 UTC (permalink / raw)
To: Peter Xu
Cc: qemu-devel, Dr . David Alan Gilbert, Markus Armbruster,
Marc-André Lureau
Peter Xu <peterx@redhat.com> writes:
> 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,
PATCH 01-03 queued. Thanks!
^ permalink raw reply [flat|nested] 29+ messages in thread