Hi Eric, Thanks for your comment on v1 and v2 patch, I will update v3 based on v1 according to your advise. Regards, Alex ----- Original Message ----- From: "Eric Blake" <eblake@redhat.com> To: "Peter Krempa" <pkrempa@redhat.com> Cc: ajia@redhat.com, libvir-list@redhat.com Sent: Tuesday, February 14, 2012 9:11:47 PM Subject: Re: [libvirt] [PATCH] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats On 02/14/2012 03:08 AM, Peter Krempa wrote:
On 02/14/2012 09:31 AM, ajia@redhat.com wrote:
From: Alex Jia<ajia@redhat.com>
Detected by valgrind. Leaks are introduced in commit 17c7795.
* python/libvirt-override.c (libvirt_virNodeGetMemoryStats): fix memory leaks and improve codes return value.
For details, please see the following link: RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=770944
Signed-off-by: Alex Jia<ajia@redhat.com> --- python/libvirt-override.c | 41 ++++++++++++++++++++++++++++++----------- 1 files changed, 30 insertions(+), 11 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 4e8a97e..ecb11ea 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2426,7 +2426,9 @@ libvirt_virNodeGetCPUStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) static PyObject * libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { - PyObject *ret; + PyObject *info, *ret;
I'd initialize ret to VIR_PY_NONE here instead of doing it multiple times later on.
I'd actually initialize to NULL, not VIR_PY_NONE. Use of VIR_PY_NONE increments the reference count on the Python None object; and if you then later return anything else, you would also have to reduce that reference count to avoid a resource leak in the form of an un-freeable python object.
This label gets called only on a error path, so I'd call it "error" instead of cleanup.
+ VIR_FREE(stats); + Py_XDECREF(key); + Py_XDECREF(val); + return NULL;
You're returning NULL instead of VIR_PY_NONE. (or the variable err)
And I think that's actually correct! Returning VIR_PY_NONE is a _valid_ python object, and library code interprets it as "I successfully called your function, but had no object to return; now you can check for the libvirt error class to see why, but there is no python exception". Returning NULL means "I encountered a python exception; you can now catch that exception". They are handled quite differently in the calling code. Returning VIR_PY_NONE should be reserved for the case where we called a libvirt API that failed (then libvirt did indeed populate a libvirt error, and there is no python exception); while returning NULL should be reserved for the case where a python glue code failed (such as inability to create a new python dictionary) or where we explicitly raised a python exception (such as when we detect an OOM situation and call PyErr_NoMemory()). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org