From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2] vnc: threaded server depends on io-thread Date: Wed, 09 Mar 2011 14:51:10 +0100 Message-ID: <4D7785CE.9020706@redhat.com> References: <4D7767C0.6060609@siemens.com> <1299676892-19246-1-git-send-email-corentin.chary@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Jan Kiszka , Peter Lieven , qemu-devel , kvm@vger.kernel.org, Anthony Liguori , Jan Kiszka To: Corentin Chary Return-path: Received: from mail-qy0-f174.google.com ([209.85.216.174]:41527 "EHLO mail-qy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932319Ab1CINvQ (ORCPT ); Wed, 9 Mar 2011 08:51:16 -0500 Received: by qyk7 with SMTP id 7so3806452qyk.19 for ; Wed, 09 Mar 2011 05:51:15 -0800 (PST) In-Reply-To: <1299676892-19246-1-git-send-email-corentin.chary@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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