[Qemu-devel] Re: [PATCH] vnc: threaded server depends on io-thread

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-03-09 11:41, 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 <jan.kiszka@web.de>
> Signed-off-by: Corentin Chary <corentin.chary@gmail.com>
> ---
>  configure           |    9 +++++++++
>  ui/vnc-jobs-async.c |    4 ++++
>  2 files changed, 13 insertions(+), 0 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..093c0d4 100644
> --- a/ui/vnc-jobs-async.c
> +++ b/ui/vnc-jobs-async.c
> @@ -260,7 +260,9 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
>          goto disconnected;
>      }
>  
> +    qemu_mutex_lock_iothread();

Doesn't this comes with a risk of an ABBA deadlock between output_mutex
and the global lock? Here you take the global lock while holding
output_mutex, but I bet there is also the other way around when vnc
services are called from the main thread or a vcpu.

>      vnc_write(job->vs, vs.output.buffer, vs.output.offset);
> +    qemu_mutex_unlock_iothread();
>  
>  disconnected:
>      /* Copy persistent encoding data */
> @@ -269,7 +271,9 @@ disconnected:
>      vnc_unlock_output(job->vs);
>  
>      if (flush) {
> +        qemu_mutex_lock_iothread();
>          vnc_flush(job->vs);
> +        qemu_mutex_unlock_iothread();
>      }
>  
>      vnc_lock_queue(queue);

The second hunk looks safe.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux