On 2011-03-09 10:58, Jan Kiszka wrote: > On 2011-03-09 10:54, Corentin Chary wrote: >> Re-reading: >> >>>> So we are calling a IOHandlerRecord::fd_write handler that is NULL. >>>> Looking at qemu_set_fd_handler2, this may happen if that function is >>>> called for an existing io-handler entry with non-NULL write handler, >>>> passing a NULL write and a non-NULL read handler. And all this without >>>> the global mutex held. >> >> When using the vnc thread, nothing in the vnc thread will never be >> called directly by an IO-handler. So I don't really how in would >> trigger this. >> And since there is a lock for accessing the output buffer (and the >> network actually), there should not be any race condition either. >> >> So the real issue, is that qemu_set_fd_handler2() is called outside of >> the main thread by those two vnc_write() and vnc_flush() calls, >> without any kind of locking. > > Yes, that's what I was referring to. > >> >>> In upstream qemu, the latter - if it exists (which is not the case in >>> non-io-thread mode). >>> In qemu-kvm, those locks are yet unused. Rather the code in qemu-kvm.c >>> implements the global lock. >> >> So there is currently no lock for that when io-thread is disabled :/. >> Spice also seems to project this kind of thing with >> qemu_mutex_lock_iothread(). >> >> Maybe qemu_mutex_lock_iothread() should also be defined when >> CONFIG_VNC_THREAD=y ? >> >>> But I'm also not sure about vnc_send_framebuffer_update. Someone has to >>> go through the threaded vnc code again, very thoroughly. It looks fragile. >> >> while vnc-thread is enabled vnc_send_framebuffer_update() will always >> call vnc_write() with csock = -1 in a temporary buffer. Check >> vnc_async_encoding_start() and vnc_async_encoding_end(), they provide >> a kind of "sandbox" that prevent the thread to write anything the >> main-thread will use. You can also see that as a "transaction": the >> thread compute the update in a temporary buffer, and only send it to >> the network (real vnc_write calls with csock correctly set) once it's >> successfully finished. >> >> The is only two functions calls that break this isolation are the two >> that I pointed out earlier. > > Probably the best way is to make vnc stop fiddling with > qemu_set_fd_handler2, specifically in threaded mode. Why does it need to > set/reset the write handler all the time? The other question is: Who's responsible for writing to the client socket in threaded mode? Only the vnc thread(s), or also other qemu threads? In the former case, just avoid setting a write handler at all. Jan
Attachment:
signature.asc
Description: OpenPGP digital signature
