[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 8/9] spice: simple display
From: |
Gerd Hoffmann |
Subject: |
Re: [Qemu-devel] [PATCH 8/9] spice: simple display |
Date: |
Thu, 19 Aug 2010 18:05:21 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.7) Gecko/20100720 Red Hat/3.1.1-1.el6 Thunderbird/3.1.1 |
Hi,
+ pthread_mutex_lock(&ssd->lock);
The locking worries me here.
It only makes sense if the spice interface_* callbacks can be invoked
from threads independently of any of the QEMU threads.
If that's the case, that means that the interface_* code is potentially
running in parallel to another QEMU thread that's holding the qemu_mutex.
Yes, interface_* code can be called back from spice server thread context.
So just acquiring a private lock in the interface_* code and then
calling into common QEMU code could result in re-entrance in interfaces
that aren't re-entrant.
I think there are no such calls.
While not really unsafe, the qemu_malloc functions are not guaranteed to
be re-entrant by the interfaces today. It's also terribly subtle to have
to rely on implicit re-entrance safety.
The underlying malloc() is re-entrant, isn't it?
pflib (which is called too) is re-entrant too.
Otherwise only private data is accessed (under lock when needed).
So in the very least, any function that gets touched by something not
carrying the qemu_mutex needs to have a comment in the definition and
declaration that the functions are required to be re-entrant. Also, the
spice-display code would benefit from clearly stating where you were
holding the qemu_mutex and where you weren't.
Yep.
+#ifdef CONFIG_SPICE
+ if (using_spice) {
+ qemu_spice_display_init(ds);
+ }
+#endif
Having spice as an independent interface to the current display_type
switching seems awkward to me.
Having remote desktop protocols as DT_something seems awkward to me. It
makes sense for the local display (being none, curses, sdl, fbdev,
whatever). For remote display protocols I see no reason why we
shouldn't have multiple of them enabled at the same time, so the user
can connect with whatever he wants. And that even in parallel to a
local display if needed.
The state the patch introduces is a bit inconsistent though. But I'd
rather drop DT_VNC instead of adding DT_SPICE.
cheers,
Gerd
- Re: [Qemu-devel] [PATCH 9/9] spice: add tablet support, (continued)
- [Qemu-devel] [PATCH 8/9] spice: simple display, Gerd Hoffmann, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, Anthony Liguori, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, malc, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, Anthony Liguori, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, malc, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, Anthony Liguori, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, malc, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, Alexander Graf, 2010/08/19
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, Gerd Hoffmann, 2010/08/25
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display,
Gerd Hoffmann <=
- Re: [Qemu-devel] [PATCH 8/9] spice: simple display, Anthony Liguori, 2010/08/19