On Wed, Oct 15, 2008 at 04:26:40PM -0400, Ben Guthro wrote:
I've been off implementing all of these changes, and I came across a point I would like to discuss, and get your opinion on.
...
In fat I think this scheme ought to let you do away with the mutex locking completely. The contract for virConnectPtr dictates that you are forbidden to use a single virConnectPtr object from more than one thread at once, so if we're queueing & dispatching events from and timeout handler, we shouldn't ever get a reentrancy/locking problem.
We potentially have a race condition for pulling data off the wire by the following functions:
call() remoteDomainEventFired()
These 2 functions lock on not the connection lock, but the private_data->lock.
Since the remoteDomainEventFired() is called from a client app via HandleImpl - it has no knowledge of that the opaque pointer being passed is a conn.
There is nothing constraining the EventImpl from making a callback while using a conn ptr in another thread.
So - if that callback happens concurrent with an explicit use of the conn ptr bad things will happen.
Our threading rule is that a single virConnectPtr must only ever be used one from thread. So if the application which registers for async events knows that it will be using the connection from a different thread than the main loop, it is supposed to open a 2nd virConnectPtr object to deal with this. This is a slightly tedious restriction to have, but I don't see a way around it other than explicitly doing the work to make it safe for a single virConnectPtr to be used from many threads. This woud thread that we add explicit locking into all the internal drivers. The work I'm doing to make QEMU / libvirtd properly threaded is a step towards this possibility, though not sufficient on its own. So if I'm understanding your scenario I think we can just document that its the app's responsibility to ensure the event loop isn't running in a separate thread from the code doing synchronous method calls. Even without events, apps like virt-manager have to handle this todo allow things like save/restore to be done in background safely. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|