On 02/14/2012 02:11 PM, Eric Blake wrote:
On 02/14/2012 03:08 AM, Peter Krempa wrote:
On 02/14/2012 09:31 AM, ajia@xxxxxxxxxx wrote:
From: Alex Jia<ajia@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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.
Yes I realized that later on :( (after some reading on creation of
python bindings :/)
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()).
I didn't notice this difference :( My review for V2 has still some
issues then.
Peter
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list