From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=40098 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PxJnK-0006en-GW for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:51:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PxJnI-00017u-Qx for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:51:18 -0500 Received: from mail-qy0-f173.google.com ([209.85.216.173]:33164) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PxJnI-00017g-H6 for qemu-devel@nongnu.org; Wed, 09 Mar 2011 08:51:16 -0500 Received: by qyk36 with SMTP id 36so4697518qyk.4 for ; Wed, 09 Mar 2011 05:51:15 -0800 (PST) Sender: Paolo Bonzini Message-ID: <4D7785CE.9020706@redhat.com> Date: Wed, 09 Mar 2011 14:51:10 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <4D7767C0.6060609@siemens.com> <1299676892-19246-1-git-send-email-corentin.chary@gmail.com> In-Reply-To: <1299676892-19246-1-git-send-email-corentin.chary@gmail.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [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: Corentin Chary Cc: kvm@vger.kernel.org, Jan Kiszka , qemu-devel , Anthony Liguori , Jan Kiszka , Peter Lieven On 03/09/2011 02:21 PM, Corentin Chary wrote: > 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); Acked-by: Paolo Bonzini for stable. For 0.15, I believe an iohandler-list lock is a better solution. Paolo