From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42077 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Om7cP-0004Wj-UO for qemu-devel@nongnu.org; Thu, 19 Aug 2010 12:05:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Om7cM-0005Ub-3p for qemu-devel@nongnu.org; Thu, 19 Aug 2010 12:05:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10109) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Om7cL-0005UV-Sn for qemu-devel@nongnu.org; Thu, 19 Aug 2010 12:05:26 -0400 Message-ID: <4C6D5641.8080004@redhat.com> Date: Thu, 19 Aug 2010 18:05:21 +0200 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 8/9] spice: simple display References: <1282221625-29501-1-git-send-email-kraxel@redhat.com> <1282221625-29501-9-git-send-email-kraxel@redhat.com> <4C6D4221.8080504@codemonkey.ws> In-Reply-To: <4C6D4221.8080504@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: qemu-devel@nongnu.org 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