> Hi,
>
> >> IMO spice-server must not call interface_client_set_capabilities
> >> when the vm is not running. After all we notify spice-server
> >> about
> >> the vm stop/start events for a reason ...
> >
> > OK, I agree that should be fixed, we can queue this until the vm
> > starts running in spice-server. But having an assert on notify in
> > qemu is also wrong - and the only way to fix it like you pointed
> > out
> > without dropping the event is to queue it as well.
> >
> > So which will it be, queue in spice or in qemu? qemu seems a
> > simpler
> > place to catch everything.
>
> When queuing in qemu you are facing the migration issue again in a
> different way: Just this time it isn't guest state, but a qxl
> register.
> Not guest visible, but still state which must be migrated over ...
OK, good point. So the assert still bothers me - it should be logged but not
asserted. I'm talking about interface_set_client_capabilities and anywhere else
that qxl_send_events can be called.
i.e.:
commit 6614a4db6b88887dd29bfd5365f38ba0fcc264ed
Author: Alon Levy <al...@redhat.com>
Date: Tue Oct 30 18:00:33 2012 +0200
hw/qxl: warn on qxl_send_events attempt if stopped
This prevents a known abort on set_client_capabilities, that should be
fixed in upstream, but it should also be checked against in qxl. Checks
every other location that qxl_send_events is eventually possibly called
(everywhere is direct except for interface_release_resource which calls
qxl_push_free_res which may call qxl_send_event).
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=870972
Signed-off-by: Alon Levy <al...@redhat.com>
diff --git a/hw/qxl.c b/hw/qxl.c
index 7b88a1e..946f5a2 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -136,6 +136,24 @@ static void qxl_reset_memslots(PCIQXLDevice *d);
static void qxl_reset_surfaces(PCIQXLDevice *d);
static void qxl_ring_set_dirty(PCIQXLDevice *qxl);
+static void spice_server_bug(PCIQXLDevice *qxl, const char *msg, ...)
+{
+ va_list ap;
+ va_start(ap, msg);
+ fprintf(stderr, "qxl-%d: spice-server bug: ", qxl->id);
+ vfprintf(stderr, msg, ap);
+ fprintf(stderr, "\n");
+ va_end(ap);
+}
+
+#define SPICE_SERVER_BUG_ONCE(qxl, msg, ...) { \
+ static int called; \
+ if (!called) { \
+ called = 1; \
+ spice_server_bug(qxl, msg, __VA_ARGS__); \
+ } \
+}
+
void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
{
trace_qxl_set_guest_bug(qxl->id);
@@ -600,6 +618,10 @@ static int interface_get_command(QXLInstance *sin, struct
QXLCommandExt *ext)
int notify, ret;
trace_qxl_ring_command_check(qxl->id, qxl_mode_to_string(qxl->mode));
+ if (!qemu_spice_display_is_running(&qxl->ssd)) {
+ SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+ return false;
+ }
switch (qxl->mode) {
case QXL_MODE_VGA:
@@ -722,6 +744,11 @@ static void interface_release_resource(QXLInstance *sin,
return;
}
+ if (!qemu_spice_display_is_running(&qxl->ssd)) {
+ SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+ return;
+ }
+
/*
* ext->info points into guest-visible memory
* pci bar 0, $command.release_info
@@ -761,6 +788,10 @@ static int interface_get_cursor_command(QXLInstance *sin,
struct QXLCommandExt *
trace_qxl_ring_cursor_check(qxl->id, qxl_mode_to_string(qxl->mode));
+ if (!qemu_spice_display_is_running(&qxl->ssd)) {
+ SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+ return false;
+ }
switch (qxl->mode) {
case QXL_MODE_COMPAT:
case QXL_MODE_NATIVE:
@@ -862,6 +893,11 @@ static void interface_async_complete_io(PCIQXLDevice *qxl,
QXLCookie *cookie)
"qxl: %s: error: current_async = %d != %"
PRId64 " = cookie->io\n", __func__, current_async, cookie->io);
}
+ if (!qemu_spice_display_is_running(&qxl->ssd)) {
+ SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+ return;
+ }
+
switch (current_async) {
case QXL_IO_MEMSLOT_ADD_ASYNC:
case QXL_IO_DESTROY_PRIMARY_ASYNC:
@@ -963,6 +999,10 @@ static void interface_set_client_capabilities(QXLInstance
*sin,
runstate_check(RUN_STATE_POSTMIGRATE)) {
return;
}
+ if (!qemu_spice_display_is_running(&qxl->ssd)) {
+ SPICE_SERVER_BUG_ONCE(qxl, "%s: guest stopped", __func__);
+ return;
+ }
qxl->shadow_rom.client_present = client_present;
memcpy(qxl->shadow_rom.client_capabilities, caps, sizeof(caps));
>
> cheers,
> Gerd
>
>
>
>
