Bug 994731

(In reply to mnguyen from comment #0)

> 
> Version-Release number of selected component (if applicable):
> ------------------------------------------------------------
> Linux 2.6.18-164.15.1.el5

What's needed here is the version of libvirt, not the kernel, that you tested.  Furthermore, have you considered upgrading to RHEL 6?  There have been lots of memory leaks plugged in the meantime, and it may be easier for you to upgrade than to take the risk that libvirt developers can figure out which particular leak you are seeing, and determine whether it can even be backported without risking regressions on the backport.

The libvirt info:

Name       : libvirt
Arch       : x86_64
Version    : 0.6.3
Release    : 20.1.el5_4
Size       : 7.1 M


Sorry it is not possible for us to upgrade to RHEL 6 at this point since we are on the verge of a major release of our product based on RHEL 5.  

I will try to do some test on RHEL 6, but just for experimentation only.

Thanks!

Even trying libvirt-0.8.2-29.el5 from RHEL 5.10 is better than sticking with the ancient history of libvirt-0.6.3-20.1.el5_4 from RHEL 5.4; while you may be unable to upgrade to RHEL 6, it should certainly be easier to upgrade to the latest RHEL 5.

(In reply to mnguyen from comment #2)
> Sorry it is not possible for us to upgrade to RHEL 6 at this point since we
> are on the verge of a major release of our product based on RHEL 5.  
> 
> I will try to do some test on RHEL 6, but just for experimentation only.

Please test against 5.10 also and let us know the result.

I did a test using the attached program in RHEL6 environment.  Here is the libvirt info:

Name        : libvirt
Arch        : x86_64
Version     : 0.10.2
Release     : 18.el6
Size        : 5.6 M
Repo        : installed
From repo   : anaconda-CentOS-201303020151.x86_64
Summary     : Library providing a simple virtualization API
URL         : http://libvirt.org/
License     : LGPLv2+
Description : Libvirt is a C toolkit to interact with the virtualization capabilities
            : of recent versions of Linux (and other OSes). The main package includes
            : the libvirtd server exporting the virtualization support.

------------------------------------------

The results using the original attached program:

1. The call to virConnectClose() returned a "1".  I have no idea why there was still a reference to the connection in the logic.  Has something changed in libvirt API in RHEL 6?

2. The while loop did not go very far.  After around 20 iterations with no problem, I then got an error message:
libvir: XML-RPC error : Cannot recv data: Connection reset by peer

-------------------------------------------

So there is something changed in the way virConnectOpen and virConnectClose work in RHEL6.  I decided to remove the virConnectOpen and virConnectClose from the while loop, so basically the while loop now only tests 2 functions virDomainLookupByName and virDomainGetInfo.  Here is the modified code:

int
main(int argc, char **argv)
{
    virConnectPtr vp;
    virDomainPtr dom = NULL;
    virDomainInfo info;
    int ret;

    vp = virConnectOpen(NULL);
    virInitialize();

    while (1) {
        dom = virDomainLookupByName(vp, "mytestdomain");
        if (!dom) {
            printf("Fail look up\n");
            continue;
        }

        memset(&info, 0, sizeof(virDomainInfo));
        virDomainGetInfo(dom, &info);
        printf("state=%d\n", info.state);
        printf("maxMem=%d\n", info.maxMem);
        printf("memory=%d\n", info.memory);
        printf("nrVirtCpu=%d\n", info.nrVirtCpu);
        printf("cpuTime=%lld\n", info.cpuTime);
    }
    virConnectClose(vp);  // never get here anyway
    return 0;
}

Using the above modified program, I got major memory leak too.  
Since the libvirt API does not specify that the caller must free the return object from virDomainLookupByName, let me know if I need to free it by some other procedure.


Thus, if RHEL6 still has memory leak issue, should I try the libvirt-0.8.2-29.el5 from RHEL 5.10?

(In reply to mnguyen from comment #5)
> Using the above modified program, I got major memory leak too.  
> Since the libvirt API does not specify that the caller must free the return
> object from virDomainLookupByName, let me know if I need to free it by some
> other procedure.

You do need to free it by calling virDomainFree when you are done with it, and indeed http://libvirt.org/html/libvirt-libvirt.html#virDomainLookupByName does not mention that.  Thanks for reporting this, and this BZ will track getting the docs updated.

Pretty much ALL of the libvirt API that return a vir*Ptr require the caller to later call vir*Free when they are done with that object.  It's not just DomainLookupByName that has this convention (ie. if we update the docs to mention that returned objects need to be freed, we need to update a LOT of functions; the alternative is documenting the conventions once up front).  But the conventions have been unchanged since old releases.  What DID change between your older version and the RHEL6 virConnectClose is that we got smarter about reference counting, where a connection is no longer closed if you leak a reference (in RHEL5, once the connection closes, your attempts to use the leaked reference will segfault; in RHEL6, the connection remains open until all references are gone so you can't segfault).  The reason your loop stopped after 20 connect/close pairs is that the default limit on open connections is 20, to prevent a denial of service (ie. libvirtd is protecting itself from your leak); you can raise the limit on number of concurrent connections, but better is to fix your code to avoid the leak in the first place.

YESSS!!  Calling virDomainFree() eliminated the memory leak problem in the test program.

This is tested in the original RHEL5.4 environment.
So we could close this bug after the document is updated to reflect the need to document the need to call virDomainFree.

Thank you for all your supports,
Minh Nguyen

(In reply to Eric Blake from comment #7)
> Pretty much ALL of the libvirt API that return a vir*Ptr require the caller
> to later call vir*Free when they are done with that object.  It's not just
> DomainLookupByName that has this convention (ie. if we update the docs to
> mention that returned objects need to be freed, we need to update a LOT of
> functions; the alternative is documenting the conventions once up front). 

I don't disagree, but I wasn't able to find a concrete statement to this effect, so I agree and we ought to have an API fundamentals section that documents all the conventions like this (maybe one exists, I just didn't find it when I looked).

Proposed upstream patch:
https://www.redhat.com/archives/libvir-list/2014-July/msg00364.html

(In reply to Dave Allan from comment #9)
> (In reply to Eric Blake from comment #7)
> > Pretty much ALL of the libvirt API that return a vir*Ptr require the caller
> > to later call vir*Free when they are done with that object.  It's not just
> > DomainLookupByName that has this convention (ie. if we update the docs to
> > mention that returned objects need to be freed, we need to update a LOT of
> > functions; the alternative is documenting the conventions once up front). 
> 
> I don't disagree, but I wasn't able to find a concrete statement to this
> effect, so I agree and we ought to have an API fundamentals section that
> documents all the conventions like this (maybe one exists, I just didn't
> find it when I looked).

The proposed patch in comment 10 only adds an up-front notice; I think we want both (an up-front notice, AND per-function notices)

I have sent an additional patch adding per-function notices:
https://www.redhat.com/archives/libvir-list/2014-July/msg00426.html

Both patches are now pushed upstream:
commit 3701b519847c391e6d0755239f7e89e9362d15f5
Author:     Ján Tomko <jtomko>
CommitDate: 2014-07-09 09:22:20 +0200

    Document the need to free vir*Ptr objects
    
    https://bugzilla.redhat.com/show_bug.cgi?id=994731

commit 3d8d18f673aecfe6d452a173cba64ec37a67d76c
Author:     Ján Tomko <jtomko>
CommitDate: 2014-07-09 09:22:20 +0200

    Document the need to free vir*Ptr objects per-function
    
    Another patch for
    https://bugzilla.redhat.com/show_bug.cgi?id=994731

git describe: v1.2.6-89-g3d8d18f

1.[root@localhost libvirt]# rpm -q libvirt
libvirt-1.2.8-10.el7.x86_64

2.[root@localhost libvirt]#  gcc -o mytest test.c -lvirt -I/usr/include/libvirt

3.[root@localhost libvirt]# valgrind --tool=memcheck --leak-check=full /home/songwang/libvirt/mytest
........
==22302== 1,663 (24 direct, 1,639 indirect) bytes in 1 blocks are definitely lost in loss record 197 of 226
==22302==    at 0x4C2BB0A: realloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==22302==    by 0x4E9256F: virReallocN (viralloc.c:245)
==22302==    by 0x4E92639: virExpandN (viralloc.c:294)
==22302==    by 0x4FDEF55: virNetClientAddProgram (virnetclient.c:881)
==22302==    by 0x4FD0C57: doRemoteOpen (remote_driver.c:995)
==22302==    by 0x4FD2E45: remoteConnectOpen (remote_driver.c:1220)
==22302==    by 0x4F6C9F9: do_open (libvirt.c:1147)
==22302==    by 0x4F6F238: virConnectOpen (libvirt.c:1317)
==22302==    by 0x400973: main (in /home/songwang/libvirt/mytest)
==22302== 
==22302== 8,315 (240 direct, 8,075 indirect) bytes in 5 blocks are definitely lost in loss record 212 of 226
==22302==    at 0x4C2B934: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==22302==    by 0x4E929C4: virAllocVar (viralloc.c:560)
==22302==    by 0x4ED4495: virObjectNew (virobject.c:193)
==22302==    by 0x4FDF343: virNetClientProgramNew (virnetclientprogram.c:80)
==22302==    by 0x4FD0BC5: doRemoteOpen (remote_driver.c:974)
==22302==    by 0x4FD2E45: remoteConnectOpen (remote_driver.c:1220)
==22302==    by 0x4F6C9F9: do_open (libvirt.c:1147)
==22302==    by 0x4F6F238: virConnectOpen (libvirt.c:1317)
==22302==    by 0x400973: main (in /home/songwang/libvirt/mytest)
==22302== 
...........

4[root@localhost libvirt]# ps -eo rss,cmd |grep mytest
 6220 ./mytest
  956 grep --color=auto mytest

5.[root@localhost libvirt]# vim /usr/share/doc/libvirt-docs-1.2.8/html/api.html 
...
269         <p>Note: functions returning vir*Ptr (like the virDomainLookup functions)
270     allocate memory which needs to be freed by the caller by the corresponding
271     vir*Free function (e.g. virDomainFree for a virDomainPtr object).
272     </p>
....

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHSA-2015-0323.html