[libvirt] Memory free in libvirt JNA

[Daniel Veillard]

  Hi Claudio,

sorry for the delay, I need to focuse one something else ATM !

On Mon, Sep 24, 2012 at 04:11:19PM +0200, Claudio Bley wrote:
> At Wed, 12 Sep 2012 20:23:28 +0800,
> Daniel Veillard wrote:
> > 
> > > I think we must provide the free functions for all the memory allocated by libvirt.
> > 
> >   Okay, can you work on making a patch ? To be honnest I'm very unlikely
> >   to have time for this in the short term,
> 
> I did notice the same thing (but was on vacation for quite some
> time). I already have a fix for virDomainGetSchedulerType in my local
> repository (see below).

  okay i see

> I'm using virFree to free the allocated memory which adds another
> level of indirection (PointerByReference). This is because JNA does
> not provide access to the C free() function by itself, AFAICS.

> -- 8< ---
> From cb58e9538afbfd7ce09c5c2a93e403a809e77113 Mon Sep 17 00:00:00 2001
> From: Claudio Bley <cbley at av-test.de>
> Date: Fri, 24 Aug 2012 11:09:49 +0200
> Subject: [PATCH] Fix memory leak for virDomainGetSchedulerType.
> 
> The string returned by virDomainGetSchedulerType must be free'd
> explicitly.
> 
> JNA docs say that we have to use a Pointer as a return type and
> free it ourselves. So, simply use virFree for that matter.
> ---
>  src/main/java/org/libvirt/Domain.java      | 13 +++++++++----
>  src/main/java/org/libvirt/jna/Libvirt.java |  6 +++++-
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/main/java/org/libvirt/Domain.java b/src/main/java/org/libvirt/Domain.java
> index 1f15ee8..fc1f665 100644
> --- a/src/main/java/org/libvirt/Domain.java
> +++ b/src/main/java/org/libvirt/Domain.java
> @@ -14,7 +14,9 @@ import org.libvirt.jna.virVcpuInfo;
>  
>  import com.sun.jna.Native;
>  import com.sun.jna.NativeLong;
> +import com.sun.jna.Pointer;
>  import com.sun.jna.ptr.IntByReference;
> +import com.sun.jna.ptr.PointerByReference;
>  
>  /**
>   * A virtual machine defined within libvirt.
> @@ -443,9 +445,11 @@ public class Domain {
>      public SchedParameter[] getSchedulerParameters() throws LibvirtException {
>          IntByReference nParams = new IntByReference();
>          SchedParameter[] returnValue = new SchedParameter[0];
> -        String scheduler = libvirt.virDomainGetSchedulerType(VDP, nParams);
> +        Pointer pScheduler = libvirt.virDomainGetSchedulerType(VDP, nParams);
>          processError();
> -        if (scheduler != null) {
> +        if (pScheduler != null) {
> +            String scheduler = pScheduler.getString(0);
> +            libvirt.virFree(new PointerByReference(pScheduler));
>              virSchedParameter[] nativeParams = new virSchedParameter[nParams.getValue()];
>              returnValue = new SchedParameter[nParams.getValue()];
>              libvirt.virDomainGetSchedulerParameters(VDP, nativeParams, nParams);
> @@ -470,10 +474,11 @@ public class Domain {
>       */
>      public String[] getSchedulerType() throws LibvirtException {
>          IntByReference nParams = new IntByReference();
> -        String returnValue = libvirt.virDomainGetSchedulerType(VDP, nParams);
> +        Pointer pScheduler = libvirt.virDomainGetSchedulerType(VDP, nParams);
>          processError();
>          String[] array = new String[1];
> -        array[0] = returnValue;
> +        array[0] = pScheduler.getString(0);
> +        libvirt.virFree(new PointerByReference(pScheduler));
>          return array;
>      }
>  
> diff --git a/src/main/java/org/libvirt/jna/Libvirt.java b/src/main/java/org/libvirt/jna/Libvirt.java
> index dc3a54b..e68d9ed 100644
> --- a/src/main/java/org/libvirt/jna/Libvirt.java
> +++ b/src/main/java/org/libvirt/jna/Libvirt.java
> @@ -7,6 +7,7 @@ import com.sun.jna.NativeLong;
>  import com.sun.jna.Pointer;
>  import com.sun.jna.ptr.IntByReference;
>  import com.sun.jna.ptr.LongByReference;
> +import com.sun.jna.ptr.PointerByReference;
>  
>  /**
>   * The libvirt interface which is exposed via JNA. The complete API is
> @@ -55,6 +56,9 @@ import com.sun.jna.ptr.LongByReference;
>   *
>   */
>  public interface Libvirt extends Library {
> +    // Memory management
> +    void virFree(PointerByReference ptr);
> +
>      // Callbacks
>      /**
>       * Callback interface for authorization
> @@ -186,7 +190,7 @@ public interface Libvirt extends Library {
>      public String virDomainGetOSType(DomainPointer virDomainPtr);
>      public int virDomainGetSchedulerParameters(DomainPointer virDomainPtr, virSchedParameter[] params,
>              IntByReference nparams);
> -    public String virDomainGetSchedulerType(DomainPointer virDomainPtr, IntByReference nparams);
> +    Pointer virDomainGetSchedulerType(DomainPointer virDomainPtr, IntByReference nparams);
>      public int virDomainGetUUID(DomainPointer virDomainPtr, byte[] uuidString);
>      public int virDomainGetUUIDString(DomainPointer virDomainPtr, byte[] uuidString);
>      public int virDomainGetVcpus(DomainPointer virDomainPtr, virVcpuInfo[] info, int maxInfo, byte[] cpumaps, int maplen);

  Okay, that makes sense. First do you have a small pointer indicating
where in JNA that kind of native deallocation must take place, since
most of the time JNA can do the marshalling all by itself ? And second
would you have an idea how to systematically detect such leaks, the
kind of loop suggested to expose the issue is nor really practical
to chase the leaks ...

  In the meantime I pushed you initial patch, thanks !

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/