diff mbox series

[PULL,08/12] spice-display: fix qemu_spice_cursor_refresh_bh locking

Message ID 20180821074509.22688-9-kraxel@redhat.com
State New
Headers show
Series [PULL,01/12] ui/sdl2: Remove the obsolete SDL_INIT_NOPARACHUTE flag | expand

Commit Message

Gerd Hoffmann Aug. 21, 2018, 7:45 a.m. UTC 
From: Paolo Bonzini <pbonzini@redhat.com>

spice-display should not call the ui/console.c functions dpy_cursor_define
and dpy_moues_set with the SimpleSpiceDisplay lock taken.  That will cause
a deadlock, because the DisplayChangeListener callbacks will take the lock
again.  It is also in general a bad idea to invoke generic callbacks with a
lock taken, because it can cause AB-BA deadlocks in the long run.  The only
thing that requires care is that the cursor may disappear as soon as the
mutex is released, so you need an extra cursor_get/cursor_put pair.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 20180720063109.4631-3-pbonzini@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-display.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

Comments

Paolo Bonzini Aug. 21, 2018, 9:45 a.m. UTC | #1 
On 21/08/2018 09:45, Gerd Hoffmann wrote:
> +    qemu_mutex_lock(&ssd->lock);
>      if (ssd->cursor) {
> +        QEMUCursor *c = ssd->cursor;
>          assert(ssd->dcl.con);
> +        cursor_get(c);
> +        qemu_mutex_unlock(&ssd->lock);
>          dpy_cursor_define(ssd->dcl.con, ssd->cursor);

Gerd,

this ssd->cursor should be "c" in the call to dpy_cursor_define.  My
apologies; please tell me if you'd like me to send a follow-up fix.

Paolo

> +        qemu_mutex_lock(&ssd->lock);
> +        cursor_put(c);
>      }
Gerd Hoffmann Aug. 21, 2018, 12:06 p.m. UTC | #2 
On Tue, Aug 21, 2018 at 11:45:20AM +0200, Paolo Bonzini wrote:
> On 21/08/2018 09:45, Gerd Hoffmann wrote:
> > +    qemu_mutex_lock(&ssd->lock);
> >      if (ssd->cursor) {
> > +        QEMUCursor *c = ssd->cursor;
> >          assert(ssd->dcl.con);
> > +        cursor_get(c);
> > +        qemu_mutex_unlock(&ssd->lock);
> >          dpy_cursor_define(ssd->dcl.con, ssd->cursor);
> 
> Gerd,
> 
> this ssd->cursor should be "c" in the call to dpy_cursor_define.  My
> apologies; please tell me if you'd like me to send a follow-up fix.

Fixed, pull v2 on the way.

cheers,
  Gerd
diff mbox series

Patch

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 46df673cd7..f1d341091a 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -450,29 +450,35 @@  void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
     qemu_mutex_unlock(&ssd->lock);
 }
 
-static void qemu_spice_cursor_refresh_unlocked(SimpleSpiceDisplay *ssd)
+void qemu_spice_cursor_refresh_bh(void *opaque)
 {
+    SimpleSpiceDisplay *ssd = opaque;
+
+    qemu_mutex_lock(&ssd->lock);
     if (ssd->cursor) {
+        QEMUCursor *c = ssd->cursor;
         assert(ssd->dcl.con);
+        cursor_get(c);
+        qemu_mutex_unlock(&ssd->lock);
         dpy_cursor_define(ssd->dcl.con, ssd->cursor);
+        qemu_mutex_lock(&ssd->lock);
+        cursor_put(c);
     }
+
     if (ssd->mouse_x != -1 && ssd->mouse_y != -1) {
+        int x, y;
         assert(ssd->dcl.con);
-        dpy_mouse_set(ssd->dcl.con, ssd->mouse_x, ssd->mouse_y, 1);
+        x = ssd->mouse_x;
+        y = ssd->mouse_y;
         ssd->mouse_x = -1;
         ssd->mouse_y = -1;
+        qemu_mutex_unlock(&ssd->lock);
+        dpy_mouse_set(ssd->dcl.con, x, y, 1);
+    } else {
+        qemu_mutex_unlock(&ssd->lock);
     }
 }
 
-void qemu_spice_cursor_refresh_bh(void *opaque)
-{
-    SimpleSpiceDisplay *ssd = opaque;
-
-    qemu_mutex_lock(&ssd->lock);
-    qemu_spice_cursor_refresh_unlocked(ssd);
-    qemu_mutex_unlock(&ssd->lock);
-}
-
 void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
 {
     graphic_hw_update(ssd->dcl.con);