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

[Peter Lieven]

Am 09.03.2011 um 12:42 schrieb Jan Kiszka:

> 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 <address@hidden>
>> Signed-off-by: Corentin Chary <address@hidden>
>> ---
>> 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.

so after all i should stay with disabled vnc-thread in qemu-kvm until there is 
a solution?

br,
peter


> 
>>     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