[libvirt] [PATCH 1/3] Domain events - primary implementation

[Daniel P. Berrange]

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