Re: [libvirt] [PATCH] Java bindings for domain events
I do not yet understand the events system, but one thing struck me:
public void handle(Domain dom, int event) {
I think that the virDomainEventType C enum shuld be represented as a
Java Enum, just as all other ENUMS are.
It does require some trickery, but makes for cleaner code (not in the
binding, but in the app using it).
I'll try to find the time understand the Events system, and provide some
in-depth feedback tomorrow.
regards
István
Daniel Veillard wrote:
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/thread-safety-for-java.html
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
Re: [libvirt] [PATCH] Java bindings for domain events
On Tue, Nov 18, 2008 at 01:16:46PM -0500, David Lively wrote: On Mon, 2008-11-17 at 22:22 +, Daniel P. Berrange wrote: On Mon, Nov 17, 2008 at 03:55:13PM -0500, David Lively wrote: Functionally this all looks fine. From a style point of view, we should keep consistency with the other virEventAddHandle func in terms of typing / param ordering. I prefer to have a typedef for the 'freefunc', even though its trivial, because I hate reading function prototypes :-) Whether we have the freefunc, before or after the 'void opaque' in the register method I don't really mind one way or the other as long as we're consistent. Having the freefunc last is probably best, since its very often just going to be NULL. Daniel Ok, here's a version with virFreeCallback as the freefunc (now called freecb) typedef. Thanks I've committed this patch now. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Java bindings for domain events
On Fri, 2008-11-14 at 12:59 -0500, David Lively wrote:
On Fri, 2008-11-14 at 17:09 +, Daniel P. Berrange wrote:
Or have the virConnectDomainEventRegister method take an extra parameter
which is a callbackvoid (*freefunc)(void*). libvirt would just invoke
that to free the opaque data chunk.
Yeah, I like this better. The dbus(?) API allows an optional
destructor (freefunc) to be specified for callback userdata. So let's
allow it to be null (in which case we obviously don't call it at remove
time).
I think we need a similar thing with the event loops APIs for timers
and file handle watches, to make it easier to free the opaque data
blob they have.
Sounds good too.
I can make the DomainEvent changes today / this weekend while working on
the Java bindings (since I need them to plug the Java leak), and submit
them on Monday (or perhaps later today, if I don't get diverted).
The attached patch implements this change (adds a freefunc arg to
virConnectDomainEventRegister and calls it on Deregister (or Close)).
It also modifies the event-test.c example to register a freefunc and
deregister callbacks when interrupted or terminated (to verify the
freefuncs are properly called).
Dave
commit 1cacb0944958dbd39f002d99721112ec2b8df7f5
Author: David Lively [EMAIL PROTECTED]
Date: Mon Nov 17 15:48:50 2008 -0500
vi-patch: events
As discussed on libvir-list, added an extra arg:
void (*freefunc)(void *opaque)
to virConnectDomainEventRegister. If non-NULL, this
function is called by virConnectDomainEventDeregister()
and passed the void *opaque argument registered with
the callback being removed.
diff --git a/examples/domain-events/events-c/event-test.c b/examples/domain-events/events-c/event-test.c
index 0a741ea..11d62c7 100644
--- a/examples/domain-events/events-c/event-test.c
+++ b/examples/domain-events/events-c/event-test.c
@@ -1,7 +1,9 @@
#include config.h
#include stdio.h
+#include stdlib.h
#include string.h
+#include signal.h
#if HAVE_SYS_POLL_H
#include sys/types.h
@@ -168,6 +170,13 @@ int myDomainEventCallback2 (virConnectPtr conn ATTRIBUTE_UNUSED,
return 0;
}
+static void myFreeFunc(void *opaque)
+{
+char *str = opaque;
+printf(%s: Freeing [%s]\n, __FUNCTION__, str);
+free(str);
+}
+
/* EventImpl Functions */
int myEventHandleTypeToPollEvent(virEventHandleType events)
@@ -254,15 +263,27 @@ void usage(const char *pname)
printf(%s uri\n, pname);
}
+int run = 1;
+
+static void stop(int sig)
+{
+printf(Exiting on signal %d\n, sig);
+run = 0;
+}
+
+
int main(int argc, char **argv)
{
-int run=1;
int sts;
+struct sigaction action_stop = {
+.sa_handler = stop
+};
if(argc 1 STREQ(argv[1],--help)) {
usage(argv[0]);
return -1;
}
+
virEventRegisterImpl( myEventAddHandleFunc,
myEventUpdateHandleFunc,
myEventRemoveHandleFunc,
@@ -277,11 +298,16 @@ int main(int argc, char **argv)
return -1;
}
+sigaction(SIGTERM, action_stop, NULL);
+sigaction(SIGINT, action_stop, NULL);
+
DEBUG0(Registering domain event cbs);
/* Add 2 callbacks to prove this works with more than just one */
-virConnectDomainEventRegister(dconn, myDomainEventCallback1, NULL);
-virConnectDomainEventRegister(dconn, myDomainEventCallback2, NULL);
+virConnectDomainEventRegister(dconn, myDomainEventCallback1,
+ strdup(callback 1), myFreeFunc);
+virConnectDomainEventRegister(dconn, myDomainEventCallback2,
+ strdup(callback 2), myFreeFunc);
while(run) {
struct pollfd pfd = { .fd = h_fd,
@@ -315,9 +341,15 @@ int main(int argc, char **argv)
}
+DEBUG0(Deregistering event handlers);
+virConnectDomainEventDeregister(dconn, myDomainEventCallback1);
+virConnectDomainEventDeregister(dconn, myDomainEventCallback2);
+
+DEBUG0(Closing connection);
if( dconn virConnectClose(dconn)0 ) {
printf(error closing\n);
}
+
printf(done\n);
return 0;
}
diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h
index d1bb154..c56d272 100644
--- a/include/libvirt/libvirt.h
+++ b/include/libvirt/libvirt.h
@@ -1095,7 +1095,8 @@ typedef int (*virConnectDomainEventCallback)(virConnectPtr conn,
int virConnectDomainEventRegister(virConnectPtr conn,
virConnectDomainEventCallback cb,
- void *opaque);
+ void *opaque,
+ void (*freefunc)(void *));
int virConnectDomainEventDeregister(virConnectPtr conn,
virConnectDomainEventCallback cb);
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
index 0ee657a..6a63ef4 100644
--- a/include/libvirt/libvirt.h.in
+++
Re: [libvirt] [PATCH] Java bindings for domain events
On Mon, Nov 17, 2008 at 03:55:13PM -0500, David Lively wrote: On Fri, 2008-11-14 at 12:59 -0500, David Lively wrote: On Fri, 2008-11-14 at 17:09 +, Daniel P. Berrange wrote: Or have the virConnectDomainEventRegister method take an extra parameter which is a callbackvoid (*freefunc)(void*). libvirt would just invoke that to free the opaque data chunk. ???Yeah, I like this better. The dbus(?) API allows an optional destructor (freefunc) to be specified for callback userdata. So let's allow it to be null (in which case we obviously don't call it at remove time). I think we need a similar thing with the event loops APIs for timers and file handle watches, to make it easier to free the opaque data blob they have. Sounds good too. I can make the DomainEvent changes today / this weekend while working on the Java bindings (since I need them to plug the Java leak), and submit them on Monday (or perhaps later today, if I don't get diverted). The attached patch implements this change (adds a freefunc arg to virConnectDomainEventRegister and calls it on Deregister (or Close)). It also modifies the event-test.c example to register a freefunc and deregister callbacks when interrupted or terminated (to verify the freefuncs are properly called). Functionally this all looks fine. From a style point of view, we should keep consistency with the other virEventAddHandle func in terms of typing / param ordering. I prefer to have a typedef for the 'freefunc', even though its trivial, because I hate reading function prototypes :-) Whether we have the freefunc, before or after the 'void opaque' in the register method I don't really mind one way or the other as long as we're consistent. Having the freefunc last is probably best, since its very often just going to be NULL. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] Java bindings for domain events
On Fri, 2008-11-14 at 17:09 +, Daniel P. Berrange wrote:
On Fri, Nov 14, 2008 at 12:00:10PM -0500, David Lively wrote:
+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 ...
Or have the virConnectDomainEventRegister method take an extra parameter
which is a callbackvoid (*freefunc)(void*). libvirt would just invoke
that to free the opaque data chunk.
Yeah, I like this better. The dbus(?) API allows an optional
destructor (freefunc) to be specified for callback userdata. So let's
allow it to be null (in which case we obviously don't call it at remove
time).
I think we need a similar thing with the event loops APIs for timers
and file handle watches, to make it easier to free the opaque data
blob they have.
Sounds good too.
I can make the DomainEvent changes today / this weekend while working on
the Java bindings (since I need them to plug the Java leak), and submit
them on Monday (or perhaps later today, if I don't get diverted).
Are you going to make the event impl changes?
Thanks,
Dave
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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/thread-safety-for-java.html
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) {
+
[libvirt] [PATCH] Java bindings for domain events
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.
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)[**].
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. 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.]
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.
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??
** 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 ..
Cheers,
Dave
diff --git a/EventTest.java b/EventTest.java
new file mode 100644
index 000..dc01f8b
--- /dev/null
+++ 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();
+ }
+}
+}
diff --git a/src/Makefile.am b/src/Makefile.am
index 5200c1d..a265de9 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -7,9 +7,11 @@ java_libvirt_source_files = \
org/libvirt/ConnectAuthDefault.java \
org/libvirt/ConnectAuth.java \
org/libvirt/DomainBlockStats.java \
+ org/libvirt/DomainEventListener.java \
org/libvirt/DomainInfo.java \
org/libvirt/DomainInterfaceStats.java \
org/libvirt/Domain.java \
+ org/libvirt/EventImpl.java \
org/libvirt/ErrorException.java \
org/libvirt/Error.java \
org/libvirt/Network.java \
diff --git a/src/jni/Makefile.am b/src/jni/Makefile.am
index c894024..829298a 100644
--- a/src/jni/Makefile.am
+++ b/src/jni/Makefile.am
@@ -5,6 +5,7 @@ GENERATED = \
org_libvirt_Domain_CreateFlags.h \
org_libvirt_Domain_MigrateFlags.h \
org_libvirt_Domain_XMLFlags.h \
+ org_libvirt_EventImpl.h \
org_libvirt_StoragePool_BuildFlags.h \
org_libvirt_StoragePool_DeleteFlags.h \
org_libvirt_StoragePool.h \
@@ -25,6 +26,9 @@ org_libvirt_Network.h: $(JAVA_CLASS_ROOT)/org/libvirt/Network.class
org_libvirt_Domain.h org_libvirt_Domain_CreateFlags.h org_libvirt_Domain_MigrateFlags.h org_libvirt_Domain_XMLFlags.h : $(JAVA_CLASS_ROOT)/org/libvirt/Domain.class
$(JAVAH) -classpath $(JAVA_CLASS_ROOT) org.libvirt.Domain
+org_libvirt_EventImpl.h : $(JAVA_CLASS_ROOT)/org/libvirt/EventImpl.class
+ $(JAVAH) -classpath $(JAVA_CLASS_ROOT) org.libvirt.EventImpl
+
org_libvirt_StoragePool.h org_libvirt_StoragePool_BuildFlags.h org_libvirt_StoragePool_DeleteFlags.h : $(JAVA_CLASS_ROOT)/org/libvirt/StoragePool.class
$(JAVAH) -classpath $(JAVA_CLASS_ROOT) org.libvirt.StoragePool
@@ -36,6 +40,7 @@ libvirt_jni_la_SOURCES = \
org_libvirt_Network.c \
org_libvirt_Connect.c \
org_libvirt_Domain.c \
+ org_libvirt_EventImpl.c \
org_libvirt_StoragePool.c \
org_libvirt_StorageVol.c \
generic.h \
diff --git a/src/jni/org_libvirt_Connect.c b/src/jni/org_libvirt_Connect.c
index cbf437c..3107159 100644
--- a/src/jni/org_libvirt_Connect.c
+++
