On 06/18/2012 08:22 AM, Peter Krempa wrote:
On 06/15/12 06:18, Eric Blake wrote:
This adds support for the new virDomainListAllSnapshots (a domain function) and virDomainSnapshotListAllChildren (a snapshot function) to the libvirt-python bindings. The implementation is done manually as the generator does not support wrapping lists of C pointers into python objects.
+ for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
Sadly, this code pattern doesn't save us from leaking memory: The PyCapsule objects, that are used to wrap libvirt pointers lack a destructor, so if this whole function fails, calling Py_DECREF() on the resulting output list doesn't save us from leaking all the processed references that were stored in the List. Later on, when the list of PyCapsules is converted into actual python-libvirt objects, those objects contain destructors that dispose the pointers properly.
A workaround would be not to NULL members of the snap array and on successful end of the loop just set c_retval to 0 so that the cleanup loop is not called. But this still is not perfect.
To fix all possibilities of leaking these pointers, we will need to provide destructor callbacks for PyCapsules that wrap libvirt pointers and increase the reference count, when the Capsule object is created.
This does not apply to libvirt_virDomainSnapshotListNames and others as strings are wrapped by python wrappers that (I assume) have destructors, but does apply on my domain listing bindings.
Sounds like a global cleanup worth doing over multiple affected functions at once...
ACK, as the code follows a pattern, but memory leaks are possible yet very unlikely.
so I pushed it as-is, leaving the PyCapsule fix for later. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org