[libvirt] libvirt-java storage support and refactoring

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/