On Wed, 2008-11-12 at 10:22 +0100, Daniel Veillard wrote: > On Fri, Nov 07, 2008 at 03:46:37PM -0500, David Lively wrote: > > It shouldn't be hard to make this thread-safe using Java synchronized > > methods and statements, but I haven't done that yet. Should I?? > > Well if we can, we probably should, yes. I found a bit of explanations > at > http://research.operationaldynamics.com/blogs/andrew/software/java-gnome/thread-safety-for-java.html Right. Mostly it consists of declaring certain methods as "synchronized", and wrapping other code in "synchronized" blocks. The more I think about it, the more I'm convinced this should be done. > if we can do that entierely in the java part of the bindings, then yes > that looks a really good idea. We just need to make sure locking is at > the connection granularity, not at the library level to not force > applications monitoring a bunch of nodes to serialize all their access > on a single lock. I'll go ahead and do this with per-connection locks. I suppose this means I need to make the various Domain/Network/StoragePool/StorageVol methods synchronize on the connection as well .... so this will involve a fair amount of churn in the Java code. I'll try to implement it over the weekend and submit something by Monday. > > +++ b/EventTest.java > > @@ -0,0 +1,35 @@ > > +import org.libvirt.*; > > + > > +class TestDomainEventListener implements DomainEventListener { > Can we move this under src/ ... along test.java ? Sure. > > > +++ b/src/jni/org_libvirt_Connect.c > > @@ -1,3 +1,4 @@ > > +#include <jni.h> > [...] > > +static JavaVM *jvm; > > + > > +jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved) { > > + jvm = vm; > > + return JNI_VERSION_1_4; > > +} > > Hum, what is this ? This is a little hook that gets called when the libvirt JNI bindings are loaded. I use it to grab the JavaVM pointer, which we'll need later in domainEventCallback. domainEventCallback is called asynchronously from the libvirt eventImpl. It's not a JNI call that gets a JNIEnv passed to it, but it needs the JNIEnv in order to make the Java callbacks. We need the JavaVM pointer so that we can get the JNIEnv via (*jvm)->GetEnv() (see below). > > +static int domainEventCallback(virConnectPtr conn, virDomainPtr dom, > > + int event, void *opaque) > > +{ > > + jobject conn_obj = (jobject)opaque; > > + JNIEnv * env; > > + jobject dom_obj; > > + jclass dom_cls, conn_cls; > > + jmethodID ctorId, methId; > > + > > + // Invoke conn.fireDomainEventCallbacks(dom, event) > > + > > + if ((*jvm)->GetEnv(jvm, (void **)&env, JNI_VERSION_1_4) != JNI_OK || env == NULL) { > > + printf("error getting JNIEnv\n"); > > + return -1; > > + } > > + > > + dom_cls = (*env)->FindClass(env, "org/libvirt/Domain"); > > + if (dom_cls == NULL) { > > + printf("error finding class Domain\n"); > > + return -1; > > + } > > + > > + ctorId = (*env)->GetMethodID(env, dom_cls, "<init>", "(Lorg/libvirt/Connect;J)V"); > > + if (ctorId == NULL) { > > + printf("error finding Domain constructor\n"); > > + return -1; > > + } > > + > > + dom_obj = (*env)->NewObject(env, dom_cls, ctorId, conn_obj, dom); > > + if (dom_obj == NULL) { > > + printf("error constructing Domain\n"); > > + return -1; > > + } > > + > > + conn_cls = (*env)->FindClass(env, "org/libvirt/Connect"); > > + if (conn_cls == NULL) { > > + printf("error finding class Connect\n"); > > + return -1; > > + } > > + > > + methId = (*env)->GetMethodID(env, conn_cls, "fireDomainEventCallbacks", "(Lorg/libvirt/Domain;I)V"); > > + if (methId == NULL) { > > + printf("error finding Connect.fireDomainEventCallbacks\n"); > > + return -1; > > + } > > + > > + (*env)->CallVoidMethod(env, conn_obj, methId, dom_obj, event); > > + > > + return 0; > > +} > > +JNIEXPORT void JNICALL Java_org_libvirt_Connect_registerForDomainEvents > > +(JNIEnv *env, jobject obj, jlong VCP){ > > + // TODO: Need to DeleteGlobalRef(obj) when deregistering for callbacks. > > + // But then need to track global obj per Connect object. > > Hum, that's a bit nasty. Can we make sure we can plug the leaks > without having to change the APIs, that would be a bummer... Yeah. It's really not acceptable as is. The easiest solution (as you hint) is changing the API so virConnectDomainEventDeregister returns the void * registered with that callback. That would (of course) be my preference. What do you think? That API hasn't been released quite yet ... > > diff --git a/src/org/libvirt/Connect.java b/src/org/libvirt/Connect.java > > index 4271937..7ccaecd 100644 > > --- a/src/org/libvirt/Connect.java > > +++ b/src/org/libvirt/Connect.java > [...] > > public class Connect { > > > > + static public EventImpl eventImpl; > > + > > // Load the native part > > static { > > System.loadLibrary("virt_jni"); > > _virInitialize(); > > + eventImpl = new EventImpl(); > > } > > > > okay, that's the hook. > Another possibility might be to use a new creation method for > Connect.new() taking an extra EventImpl class implementation. But remember EventImpl is global, not per-Connect. > > diff --git a/src/org/libvirt/EventImpl.java b/src/org/libvirt/EventImpl.java > > new file mode 100644 > > index 0000000..1868fe3 > > --- /dev/null > > +++ b/src/org/libvirt/EventImpl.java > > @@ -0,0 +1,31 @@ > > +package org.libvirt; > > + > > +import java.util.HashMap; > > + > > +// While EventImpl does implement Runnable, don't declare that until > > +// the we add concurrency control for the libvirt Java bindings and > > +// the EventImpl callbacks. > > + > > +final public class EventImpl { > > Hum, why final ? Oops. No reason -- left over from some other experiment. -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list