On Tue, 2008-11-18 at 17:05 +0000, Daniel P. Berrange wrote: > On Tue, Nov 18, 2008 at 11:18:38AM -0500, David Lively wrote: > > This patch allows the remote driver to work with an asynchronous > > EventImpl (it's the only one using an externally-supplied one), assuming > > libvirt is compiled with pthread support. (Without pthreads, this code > > is harmless in a single-threaded environment.) > > I don't like this patch, since it is only making one tiny part of the > API thread-safe, for one tiny use case. eg, if someone uses events from > the Xen driver in Java with threads, they'll crash & burn, since only > the remote driver is being protected. Currently ONLY the remote driver (and yes, soon - the xen driver, which would also need thread-safety changes) use an EventImpl supplied externally. All others use the libvirtd-provided synchronous impl. > Why doesn't the Java code simply synchronize on the virConnect object > before invoking the FD event callbacks. That will ensure another > thread has finished whatever API call it was doing Note that the Java code uses (Java's builtin) Connect-level lock to avoid concurrent calls using the same virConnect. This even applies to domain event delivery - note that Connect.fireDomainEventCallbacks is (in the Java patch) a synchronized method. A FD event callback isn't associated with a particular virConnect object, and EventImpls aren't virConnect-specific, so I can't lock "the virConnect" (see below). When we exposed virEventRegisterImpl, we said that externally-supplied event impls must not make callbacks when a virConnect is being used. If the Java EventImpl were following this rule, we wouldn't need this patch. BUT because the EventImpl can't know which virConnect (if any) is involved in a particular callback, the only way to satisfy this rule is to lock ALL Connect objects before making an EventImpl callback. This is (IMO) both way too restrictive, and (I'm a little less sure of this) not restrictive enough. (I suspect we'd find that making two callbacks concurrently can break things, even if no virConnects are in use at the time.) I think we have to allow externally supplied EventImpls to make callbacks without regard to which virConnect, etc. objects are in use, since EventImpls don't know anything about virConnect, etc. This doesn't require all of libvirt to be made thread-safe. So I view this fix (and the one necessary for the xen driver once it starts using an externally-supplied EventImpl) as providing just enough concurrency control to allow an asynchronous EventImpl, while still making the libvirt user (the Java bindings, in my case) responsible for avoiding concurrent virConnect usage. -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list