Re: [libvirt] libvirt-java storage support and refactoring
On Sun, 2008-08-03 at 05:56 -0400, Daniel Veillard wrote:
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 ?
Actually, that's not the new-style code, It's just the bugfixed one. I
have not converted that file yet.
To see the new style code, look at the *Storage*.c files.
A similar function looks like this there:
JNIEXPORT jlong JNICALL Java_org_libvirt_StoragePool__1storageVolCreateXML
(JNIEnv *env, jobject obj, jlong VSPP, jstring xmlDesc, jint flags){
return generic_CreateDefineXML_with_flags(env, obj, VSPP, xmlDesc, flags, (void*
(*)(void*, const char *, unsigned int))&virStorageVolCreateXML);
}
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.
Actually, the old-style function does static type checking, it's just
new style that suffers there.
Using macros is a great idea, that may get rid of all those nasty casts.
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