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
