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
>
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list