From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=60301 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PxJKf-0004HT-VF for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:21:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PxJKe-0007Lg-RX for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:21:41 -0500 Received: from smtp6.tech.numericable.fr ([82.216.111.42]:39707) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PxJKe-0007LL-JK for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:21:40 -0500 From: Corentin Chary Date: Wed, 9 Mar 2011 14:21:32 +0100 Message-Id: <1299676892-19246-1-git-send-email-corentin.chary@gmail.com> In-Reply-To: <4D7767C0.6060609@siemens.com> References: <4D7767C0.6060609@siemens.com> Subject: [Qemu-devel] [PATCH v2] vnc: threaded server depends on io-thread List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: kvm@vger.kernel.org, Peter Lieven , qemu-devel , Anthony Liguori , Jan Kiszka , Corentin Chary The threaded VNC servers messed up with QEMU fd handlers without any kind of locking, and that can cause some nasty race conditions. The IO-Thread provides appropriate locking primitives to avoid that. This patch makes CONFIG_VNC_THREAD depends on CONFIG_IO_THREAD, and add lock and unlock calls around the two faulty calls. Thanks to Jan Kiszka for helping me solve this issue. Cc: Jan Kiszka Signed-off-by: Corentin Chary --- The previous patch was total crap, introduced race conditions, and probably crashs on client disconnections. configure | 9 +++++++++ ui/vnc-jobs-async.c | 24 +++++++++++++++++++----- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/configure b/configure index 5513d3e..c8c1ac1 100755 --- a/configure +++ b/configure @@ -2455,6 +2455,15 @@ if test \( "$cpu" = "i386" -o "$cpu" = "x86_64" \) -a \ roms="optionrom" fi +# VNC Thread depends on IO Thread +if test "$vnc_thread" = "yes" -a "$io_thread" != "yes"; then + echo + echo "ERROR: VNC thread depends on IO thread which isn't enabled." + echo "Please use --enable-io-thread if you want to enable it." + echo + exit 1 +fi + echo "Install prefix $prefix" echo "BIOS directory `eval echo $datadir`" diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c index f596247..d0c6f61 100644 --- a/ui/vnc-jobs-async.c +++ b/ui/vnc-jobs-async.c @@ -190,6 +190,18 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local) queue->buffer = local->output; } +static void vnc_worker_lock_output(VncState *vs) +{ + qemu_mutex_lock_iothread(); + vnc_lock_output(vs); +} + +static void vnc_worker_unlock_output(VncState *vs) +{ + vnc_unlock_output(vs); + qemu_mutex_unlock_iothread(); +} + static int vnc_worker_thread_loop(VncJobQueue *queue) { VncJob *job; @@ -211,11 +223,11 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) return -1; } - vnc_lock_output(job->vs); + vnc_worker_lock_output(job->vs); if (job->vs->csock == -1 || job->vs->abort == true) { goto disconnected; } - vnc_unlock_output(job->vs); + vnc_worker_unlock_output(job->vs); /* Make a local copy of vs and switch output buffers */ vnc_async_encoding_start(job->vs, &vs); @@ -236,7 +248,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) /* output mutex must be locked before going to * disconnected: */ - vnc_lock_output(job->vs); + vnc_worker_lock_output(job->vs); goto disconnected; } @@ -255,7 +267,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue) vs.output.buffer[saved_offset + 1] = n_rectangles & 0xFF; /* Switch back buffers */ - vnc_lock_output(job->vs); + vnc_worker_lock_output(job->vs); if (job->vs->csock == -1) { goto disconnected; } @@ -266,10 +278,12 @@ disconnected: /* Copy persistent encoding data */ vnc_async_encoding_end(job->vs, &vs); flush = (job->vs->csock != -1 && job->vs->abort != true); - vnc_unlock_output(job->vs); + vnc_worker_unlock_output(job->vs); if (flush) { + qemu_mutex_lock_iothread(); vnc_flush(job->vs); + qemu_mutex_unlock_iothread(); } vnc_lock_queue(queue); -- 1.7.3.4