Re: [libvirt] [PATCH] Java bindings for domain events

On Fri, Nov 07, 2008 at 03:46:37PM -0500, David Lively wrote: > The attached patch (against libvirt-java) contains Java bindings for the > new domain event code. It works (see EventTest.java), but there's a > certain amount of hokiness regarding the EventImpl stuff that I'd like > to discuss. > In general it looks okay, but I'm really not a Java head :-\ I would feel better if István could have a look at it too ! > Unlike the C and Python interfaces, the Java interface does not > currently allow the client to supply an EventImpl. The problem is that > Java really has no way to interact with unix file descriptors so there's > no reasonable way to implement a fd-watching EventImpl in pure Java > (java.io.FileDescriptor doesn't do the trick since there's no way of > constructing one from raw (int) unix fd)[**]. > Right, I tried to check how the java Gnome guys are doing it but could not find anything in the example on how to add an external source to a gdk.main() loop ... I guess that's just against Java common coding practices. > So for now, I've had the Java bindings register an EventImpl when the > Connect class is loaded. This EventImpl is a Java class, with native > methods implementing it. > Yes that's probably the best way to map this on the API, I will try to check the syntactic details, but again I'm not a Java expert by far... > In fact, I've simply stolen (verbatim) the > EventImpl from libvirt/qemud/events.c and made the native methods call > it. [If we stick with this solution, it would obviously be better to > somehow share this code with libvirtd rather than copy it.] > different code base, and unless exporting them we're probably safer keeping a copy, maybe add a note on both side so that if someone modify the code it know it's a reference to another part... > The other tricky subject is multi-threading. For now, I've avoided it > by exposing Connect.eventImpl.run_once() and forcing the client to call > it from their "event loop". But the normal Java way of doing things > would simply run the EventImpl in a separate thread. In fact, this > EventImpl does implement Runnable, which makes it trivial to run in a > separate thread -- but I don't declare that it implements Runnable yet > because it's not safe to run in a thread while another thread might be > making libvirt calls. > I dislike the bias of Java APIs to force multi-threading. If we could avoid it at the moment I would feel safer, at least until someone who knows this stuff well could comment. Maybe it's better to not use a thread automatically at this point, program can adapt to the manual main loop addition for now, but if they are using other components which are not thread safe, it's better to not force them to manually add synchronization in their code just to cope with libvirt shelling out a thread. I don't know if my view here is realistic :-\ > 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/... 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. > ** java.nio.Channel and friends seem to be the "right" interface for > exposing abstract "selectable channels" in Java. It's just complicated > enough that I've avoided it for now. But I can look into going this way > for allowing Java to provide an EventImpl in the future .. > yeah that's scary... > +++ b/EventTest.java > @@ -0,0 +1,35 @@ > +import org.libvirt.*; > + > +class TestDomainEventListener implements DomainEventListener { > + > + String name; > + > + TestDomainEventListener(String name) { > + this.name = name; > + } > + > + public void handle(Domain dom, int event) { > + try { > + System.out.println(name + ": dom " + dom.getName() + " got event " + event); > + } catch (LibvirtException e) { > + System.out.println(e); > + System.out.println(name + ": unknown dom got event " + event); > + } > + } > +} > + > +class EventTest { > + > + public static void main(String args[]) throws LibvirtException { > + String URI = "qemu:///system"; > + if (args.length > 0) > + URI = args[0]; > + Connect conn = new Connect(URI); > + conn.domainEventRegister(new TestDomainEventListener("Test 1")); > + conn.domainEventRegister(new TestDomainEventListener("Test 2")); > + > + while (true) { > + conn.eventImpl.run_once(); > + } > + } > +} > Can we move this under src/ ... along test.java ? > +++ 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 ? > +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... > + obj = (*env)->NewGlobalRef(env, obj); > + virConnectDomainEventRegister((virConnectPtr)VCP, domainEventCallback, obj); > +} > + > +JNIEXPORT void JNICALL Java_org_libvirt_Connect_deregisterForDomainEvents > +(JNIEnv *env, jobject obj, jlong VCP){ > + virConnectDomainEventDeregister((virConnectPtr)VCP, domainEventCallback); > +} > [...] > +++ b/src/jni/org_libvirt_EventImpl.c > @@ -0,0 +1,542 @@ > +#include <stdlib.h> > +#include <poll.h> > +#include <string.h> > +#include <errno.h> > + > +#include "org_libvirt_EventImpl.h" > +#include <libvirt/libvirt.h> > + > +#define EVENT_DEBUG(fmt, ...) //do { printf("%s:%d (" fmt ")\n", __FUNCTION__, __LINE__, __VA_ARGS__); } while (0); > + > Hum ... actually all those copies are not that bad, I don't expect the memory wrappers to change, and we certainly won't export them, so ... > +// Copied from libvirt/src/memory.h: > + > +#define VIR_ALLOC_N(ptr, count) __virAllocN(&(ptr), sizeof(*(ptr)), (count)) > +#define VIR_REALLOC_N(ptr, count) __virReallocN(&(ptr), sizeof(*(ptr)), (count)) > +#define VIR_FREE(ptr) __virFree(&(ptr)) > + > +// Copied from libvirt/src/memory.c: > + > +static int __virAllocN(void *ptrptr, size_t size, size_t count) > +{ > + *(void**)ptrptr = calloc(count, size); > + if (*(void**)ptrptr == NULL) > + return -1; > + return 0; > +} > + > +static int __virReallocN(void *ptrptr, size_t size, size_t count) > +{ > + void *tmp; > + tmp = realloc(*(void**)ptrptr, size * count); > + if (!tmp && (size * count)) > + return -1; > + *(void**)ptrptr = tmp; > + return 0; > +} > + > +static void __virFree(void *ptrptr) > +{ > + free(*(void**)ptrptr); > + *(void**)ptrptr = NULL; > +} > + > + > +// START Copied from libvirt/qemud/events.c > + > [...] > +// END Copied from libvirt/qemud/events.c > that's a large chunk, but better copy it than rewrite it, so ... > + > +JNIEXPORT void JNICALL Java_org_libvirt_EventImpl_register(JNIEnv *env, jobject obj) > +{ > + virEventRegisterImpl(virEventAddHandleImpl, virEventUpdateHandleImpl, > + virEventRemoveHandleImpl, virEventAddTimeoutImpl, > + virEventUpdateTimeoutImpl, virEventRemoveTimeoutImpl); > +} > + > +JNIEXPORT jint JNICALL Java_org_libvirt_EventImpl_run_1once(JNIEnv *env, jobject obj) > +{ > + return virEventRunOnce(); > +} > 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. [...] > +++ b/src/org/libvirt/DomainEventListener.java > @@ -0,0 +1,16 @@ > +package org.libvirt; > + > +public interface DomainEventListener > + extends java.util.EventListener { > + > + static int VIR_DOMAIN_EVENT_ADDED = 0; > + static int VIR_DOMAIN_EVENT_REMOVED = 1; > + static int VIR_DOMAIN_EVENT_STARTED = 2; > + static int VIR_DOMAIN_EVENT_SUSPENDED = 3; > + static int VIR_DOMAIN_EVENT_RESUMED = 4; > + static int VIR_DOMAIN_EVENT_STOPPED = 5; > + static int VIR_DOMAIN_EVENT_SAVED = 6; > + static int VIR_DOMAIN_EVENT_RESTORED = 7; > + > + public void handle(Domain dom, int event); > +} > Okay > 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 ? > + boolean stopped = false; > + > + EventImpl() { > + register(); > + } > + > + private native void register(); > + public native int run_once(); > + > + public void run() { > + stopped = false; > + while (! stopped) { > + if (run_once() != 0) > + stopped = true; > + } > + } > + > + public void stop() { > + stopped = true; > + } > +} > Again this looks good enough for me, maybe we should add synchronization to avoid troubles and make sure we can do it in a connection by connection basis. Daniel