11 a.m.
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/...
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.