[Qemu-devel] [PATCH] spice: set qxl->ssd.running=true before telling spice to start, RHBZ #733993

[Qemu-devel] [PATCH] spice: set qxl->ssd.running=true before telling spice to start, RHBZ #733993

[Yonit Halperin]

If qxl->ssd.running=true is set after telling spice to start, the spice server
thread can call qxl_send_events while qxl->ssd.running is still false. This leads to
assert(d->ssd.running).

Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
---
Since it looks like the purpose of the assert in qxl_send_event is preventing changes
in the guest when the vm is stopped, I think it is not necessary for ssd.running to be
exactly synchronized with the spice server status, but just be true before
the spice worker starts.

 ui/spice-display.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 683d454..3224f99 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -260,11 +260,12 @@ void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
     SimpleSpiceDisplay *ssd = opaque;
 
     if (running) {
+        ssd->running = true;
         qemu_spice_start(ssd);
     } else {
         qemu_spice_stop(ssd);
+        ssd->running = false;
     }
-    ssd->running = running;
 }
 
 void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds)
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH] spice: set qxl->ssd.running=true before telling spice to start, RHBZ #733993

[Alon Levy]

On Mon, Sep 05, 2011 at 05:39:50PM +0300, Yonit Halperin wrote:
> If qxl->ssd.running=true is set after telling spice to start, the spice server
> thread can call qxl_send_events while qxl->ssd.running is still false. This leads to
> assert(d->ssd.running).
> 

Looks good.

Acked-by: Alon Levy <alevy@redhat.com>

> Signed-off-by: Yonit Halperin <yhalperi@redhat.com>
> ---
> Since it looks like the purpose of the assert in qxl_send_event is preventing changes
> in the guest when the vm is stopped, I think it is not necessary for ssd.running to be
> exactly synchronized with the spice server status, but just be true before
> the spice worker starts.
> 
>  ui/spice-display.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 683d454..3224f99 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -260,11 +260,12 @@ void qemu_spice_vm_change_state_handler(void *opaque, int running, int reason)
>      SimpleSpiceDisplay *ssd = opaque;
>  
>      if (running) {
> +        ssd->running = true;
>          qemu_spice_start(ssd);
>      } else {
>          qemu_spice_stop(ssd);
> +        ssd->running = false;
>      }
> -    ssd->running = running;
>  }
>  
>  void qemu_spice_display_init_common(SimpleSpiceDisplay *ssd, DisplayState *ds)
> -- 
> 1.7.4.4
> 
>