Re: [Qemu-devel] [PATCH 8/9] spice: simple display

[Gerd Hoffmann <kraxel@redhat.com>]

   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