3:22 a.m.
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
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/