Patchwork [v2,2/5] spice: notify on vm state change only via spice_server_vm_start/stop

login
register
mail settings
Submitter Yonit Halperin
Date Aug. 20, 2012, 1:26 p.m.
Message ID <1345469176-1675-3-git-send-email-yhalperi@redhat.com>
Download mbox | patch
Permalink /patch/178815/
State New
Headers show

Comments

Yonit Halperin - Aug. 20, 2012, 1:26 p.m.
QXLWorker->start/stop are deprecated since spice-server 0.11.2

Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
---
 ui/spice-core.c    |   44 ++++++++++++++++++++++++++++++++++++++++++++
 ui/spice-display.c |    6 ++++++
 ui/spice-display.h |    2 ++
 3 files changed, 52 insertions(+), 0 deletions(-)
Gerd Hoffmann - Aug. 21, 2012, 6:58 a.m.
Hi,

> +#if SPICE_SERVER_VERSION >= 0x000b02 /* 0.11.2 */
> +static void display_start(void)
> +{
> +    DisplayListItem *item;
> +
> +    QLIST_FOREACH(item, &display_list, link) {
> +        item->display->running = true;
> +    }
> +}

I think we should simply use a global variable for 'running' here.  It's
global state anyway.  Having this in per-display state struct buys us
nothing and adds alot of code for updating and display list maintainance.

>  void qemu_spice_vm_change_state_handler(void *opaque, int running,
>                                          RunState state)
>  {
> +#if SPICE_SERVER_VERSION < 0x000b02 /* before 0.11.2 */
>      SimpleSpiceDisplay *ssd = opaque;

> +#if SPICE_SERVER_VERSION < 0x000b02 /* before 0.11.2 */
>      qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler, &sdpy);
> +#endif

If you don't register qemu_spice_vm_change_state_handler on new
spice-server you should #ifdef the whole function, not the body only.

cheers,
  Gerd
Yonit Halperin - Aug. 21, 2012, 7:09 a.m.
Hi,
On 08/21/2012 09:58 AM, Gerd Hoffmann wrote:
>    Hi,
>
>> +#if SPICE_SERVER_VERSION>= 0x000b02 /* 0.11.2 */
>> +static void display_start(void)
>> +{
>> +    DisplayListItem *item;
>> +
>> +    QLIST_FOREACH(item,&display_list, link) {
>> +        item->display->running = true;
>> +    }
>> +}
>
> I think we should simply use a global variable for 'running' here.  It's
> global state anyway.  Having this in per-display state struct buys us
> nothing and adds alot of code for updating and display list maintainance.
>
ok, good point.
>>   void qemu_spice_vm_change_state_handler(void *opaque, int running,
>>                                           RunState state)
>>   {
>> +#if SPICE_SERVER_VERSION<  0x000b02 /* before 0.11.2 */
>>       SimpleSpiceDisplay *ssd = opaque;
>
>> +#if SPICE_SERVER_VERSION<  0x000b02 /* before 0.11.2 */
>>       qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy);
>> +#endif
>
> If you don't register qemu_spice_vm_change_state_handler on new
> spice-server you should #ifdef the whole function, not the body only.
>
qemu_spice_vm_change_state_handler is also called from qxl.c, as part of 
its own vm_change_state handler. I didn't want to add another ifdef there.

> cheers,
>    Gerd
Gerd Hoffmann - Aug. 21, 2012, 7:17 a.m.
On 08/21/12 09:09, Yonit Halperin wrote:
> Hi,
> On 08/21/2012 09:58 AM, Gerd Hoffmann wrote:
>>    Hi,
>>
>>> +#if SPICE_SERVER_VERSION>= 0x000b02 /* 0.11.2 */
>>> +static void display_start(void)
>>> +{
>>> +    DisplayListItem *item;
>>> +
>>> +    QLIST_FOREACH(item,&display_list, link) {
>>> +        item->display->running = true;
>>> +    }
>>> +}
>>
>> I think we should simply use a global variable for 'running' here.  It's
>> global state anyway.  Having this in per-display state struct buys us
>> nothing and adds alot of code for updating and display list maintainance.
>>
> ok, good point.
>>>   void qemu_spice_vm_change_state_handler(void *opaque, int running,
>>>                                           RunState state)
>>>   {
>>> +#if SPICE_SERVER_VERSION<  0x000b02 /* before 0.11.2 */
>>>       SimpleSpiceDisplay *ssd = opaque;
>>
>>> +#if SPICE_SERVER_VERSION<  0x000b02 /* before 0.11.2 */
>>>      
>>> qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler,&sdpy);
>>>
>>> +#endif
>>
>> If you don't register qemu_spice_vm_change_state_handler on new
>> spice-server you should #ifdef the whole function, not the body only.
>>
> qemu_spice_vm_change_state_handler is also called from qxl.c, as part of
> its own vm_change_state handler. I didn't want to add another ifdef there.

Fair point, but then you can drop the #ifdef for the
qemu_add_vm_change_state_handler call too.

cheers,
  Gerd

Patch

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 32de1f1..97b45f8 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -37,6 +37,7 @@ 
 #include "migration.h"
 #include "monitor.h"
 #include "hw/hw.h"
+#include "spice-display.h"
 
 /* core bits */
 
@@ -55,6 +56,16 @@  struct SpiceTimer {
 };
 static QTAILQ_HEAD(, SpiceTimer) timers = QTAILQ_HEAD_INITIALIZER(timers);
 
+typedef struct DisplayListItem DisplayListItem;
+
+struct DisplayListItem {
+    SimpleSpiceDisplay *display;
+    QLIST_ENTRY(DisplayListItem) link;
+};
+static QLIST_HEAD(, DisplayListItem) display_list =
+    QLIST_HEAD_INITIALIZER(display_list);
+
+
 static SpiceTimer *timer_add(SpiceTimerFunc func, void *opaque)
 {
     SpiceTimer *timer;
@@ -545,14 +556,36 @@  static int add_channel(const char *name, const char *value, void *opaque)
     return 0;
 }
 
+#if SPICE_SERVER_VERSION >= 0x000b02 /* 0.11.2 */
+static void display_start(void)
+{
+    DisplayListItem *item;
+
+    QLIST_FOREACH(item, &display_list, link) {
+        item->display->running = true;
+    }
+}
+
+static void display_stop(void)
+{
+    DisplayListItem *item;
+
+    QLIST_FOREACH(item, &display_list, link) {
+        item->display->running = false;
+    }
+}
+#endif
+
 static void vm_change_state_handler(void *opaque, int running,
                                     RunState state)
 {
 #if SPICE_SERVER_VERSION >= 0x000b02 /* 0.11.2 */
     if (running) {
+        display_start();
         spice_server_vm_start(spice_server);
     } else {
         spice_server_vm_stop(spice_server);
+        display_stop();
     }
 #endif
 }
@@ -754,6 +787,17 @@  int qemu_spice_add_interface(SpiceBaseInstance *sin)
         spice_server = spice_server_new();
         spice_server_init(spice_server, &core_interface);
     }
+
+    if (strcmp(sin->sif->type, SPICE_INTERFACE_QXL) == 0) {
+        QXLInstance *qxl;
+        DisplayListItem *item;
+
+        qxl = container_of(sin, QXLInstance, base);
+        item = g_malloc0(sizeof(*item));
+        item->display = container_of(qxl, SimpleSpiceDisplay, qxl);
+
+        QLIST_INSERT_HEAD(&display_list, item, link);
+    }
     return spice_server_add_interface(spice_server, sin);
 }
 
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 3e8f0b3..073a354 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -126,6 +126,7 @@  void qemu_spice_wakeup(SimpleSpiceDisplay *ssd)
     ssd->worker->wakeup(ssd->worker);
 }
 
+#if SPICE_SERVER_VERSION < 0x000b02 /* before 0.11.2 */
 void qemu_spice_start(SimpleSpiceDisplay *ssd)
 {
     trace_qemu_spice_start(ssd->qxl.id);
@@ -137,6 +138,7 @@  void qemu_spice_stop(SimpleSpiceDisplay *ssd)
     trace_qemu_spice_stop(ssd->qxl.id);
     ssd->worker->stop(ssd->worker);
 }
+#endif
 
 static SimpleSpiceUpdate *qemu_spice_create_update(SimpleSpiceDisplay *ssd)
 {
@@ -272,6 +274,7 @@  void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
 void qemu_spice_vm_change_state_handler(void *opaque, int running,
                                         RunState state)
 {
+#if SPICE_SERVER_VERSION < 0x000b02 /* before 0.11.2 */
     SimpleSpiceDisplay *ssd = opaque;
 
     if (running) {
@@ -281,6 +284,7 @@  void qemu_spice_vm_change_state_handler(void *opaque, int running,
         qemu_spice_stop(ssd);
         ssd->running = false;
     }
+#endif
 }
 
 void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds)
@@ -518,7 +522,9 @@  void qemu_spice_display_init(DisplayState *ds)
     qemu_spice_add_interface(&sdpy.qxl.base);
     assert(sdpy.worker);
 
+#if SPICE_SERVER_VERSION < 0x000b02 /* before 0.11.2 */
     qemu_add_vm_change_state_handler(qemu_spice_vm_change_state_handler, &sdpy);
+#endif
     qemu_spice_create_host_memslot(&sdpy);
     qemu_spice_create_host_primary(&sdpy);
 }
diff --git a/ui/spice-display.h b/ui/spice-display.h
index 12e50b6..cf7a8e7 100644
--- a/ui/spice-display.h
+++ b/ui/spice-display.h
@@ -129,5 +129,7 @@  void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t id,
 void qemu_spice_destroy_primary_surface(SimpleSpiceDisplay *ssd,
                                         uint32_t id, qxl_async_io async);
 void qemu_spice_wakeup(SimpleSpiceDisplay *ssd);
+#if SPICE_SERVER_VERSION < 0x000b02 /* before 0.11.2 */
 void qemu_spice_start(SimpleSpiceDisplay *ssd);
 void qemu_spice_stop(SimpleSpiceDisplay *ssd);
+#endif