Re: [libvirt] libvirt-java storage support and refactoring
On Sat, Aug 02, 2008 at 05:42:36PM +0200, Tóth István wrote:
Hello!
Welcome back :-)
This patch contains the following:
- The complete storage handling API
- Fixing memory leaks in the VirConnect JNI implementation
Very cool !
I've written the new classes using a new approach.
I've found that libvirt for the most part has a very perdicitble and
repetitive API (great design!), and as a result I've found myself
copying the same code over and over again.
I've decided to make generic JNI functions, that can handle multiple
libvirt functions with function pointers.
The generic functions are in generic.c and they are used extensively in
the new Storage JNI implementation.
I'd like to have your input on this architecture, my current plan is to
refactor all trivial JNI functions to use these generics, unless there
are objections.
The positive aspects of the new architecture:
- No code duplication, one generic function fix affects all similar
functions
- Less code
The negative aspects:
- Ugly syntax (but JNI is ugly enough already)
- Easier to make errors in JNI code due to function type casts.
I think that the benefits outweigh the negatives, esepecialy when we
start cleaning up memory allocation, 64/32 bit cleannes stuff,
threading, it will have to be done in one function, instead of 3 or 30
cut'n'pasted ones, scattered in 5 files.
Hum, I'm missing something jst by looking at the patch
JNIEXPORT jlong JNICALL
Java_org_libvirt_Connect__1virNetworkCreateXML
- (JNIEnv *env, jobject obj, jlong VCP, jstring xmlDesc){
- return (jlong)virNetworkCreateXML((virConnectPtr)VCP, (*env)->GetStringUTFChars(env,
xmlDesc, NULL));
+ (JNIEnv *env, jobject obj, jlong VCP, jstring j_xmlDesc){
+ const char *xmlDesc=(*env)->GetStringUTFChars(env, j_xmlDesc, NULL);
+ jlong retval = (jlong)virNetworkCreateXML((virConnectPtr)VCP, xmlDesc);
+ (*env)->ReleaseStringUTFChars(env, j_xmlDesc, xmlDesc);
+ return retval;
}
How is it smaller code ?
It seems to be that the old code didn't ever tried to free allocated strings
and the new one does, which is the explanation of the code grows. I would side
with Chris on the usage of macros instead of call like this. There is 2 reasons
one is the readability, but also the static type checking. Bindings code is
as you noticed boring, repetitive, and a mistake there is easy. My approach
was to automate (as much as possible) the bindings for python based on the
XML description. But that's incomplete, manual bindings are fine, the best is
to try to avoid mistake due to repetitive nature of the code.
I would try an approach which allows the compiler to do the full type checking,
I think that's the best way to avoid some of the mistakes, it won't prevent
forgetting a piece of deallocation for example, but will catch mistakes in
arguments at least.
But I'm not opposed to a less flexible approach, I just try to weight the
pros and cons :-)
thanks a lot !
Daniel
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/