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/thr...
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.