[libvirt] [PATCH] Java bindings for domain events

[David Lively]

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.