[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]