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(a)redhat.com wrote: >> From: Alex Jia<ajia(a)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(a)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()).

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list