[libvirt] [PATCHv2] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats
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 | 43 +++++++++++++++++++++++++++++-------------- 1 files changed, 29 insertions(+), 14 deletions(-) diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 4e8a97e..594aae6 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 *ret = VIR_PY_NONE; + PyObject *key = NULL; + PyObject *val = NULL; PyObject *pyobj_conn; virConnectPtr conn; unsigned int flags; @@ -2442,32 +2444,45 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) c_retval = virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, flags); LIBVIRT_END_ALLOW_THREADS; if (c_retval < 0) - return VIR_PY_NONE; + return ret; if (nparams) { if (VIR_ALLOC_N(stats, nparams) < 0) - return VIR_PY_NONE; + return PyErr_NoMemory(); LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virNodeGetMemoryStats(conn, cellNum, stats, &nparams, flags); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - VIR_FREE(stats); - return VIR_PY_NONE; - } - } - if (!(ret = PyDict_New())) { - VIR_FREE(stats); - return VIR_PY_NONE; + if (c_retval < 0) + goto error; } + if (!(ret = PyDict_New())) + goto error; + for (i = 0; i < nparams; i++) { - PyDict_SetItem(ret, - libvirt_constcharPtrWrap(stats[i].field), - libvirt_ulonglongWrap(stats[i].value)); + key = libvirt_constcharPtrWrap(stats[i].field); + val = libvirt_ulonglongWrap(stats[i].value); + + if (!key || !val) + goto error; + + if (PyDict_SetItem(ret, key, val) < 0) { + Py_DECREF(ret); + goto error; + } + + Py_DECREF(key); + Py_DECREF(val); } VIR_FREE(stats); return ret; + +error: + VIR_FREE(stats); + Py_XDECREF(key); + Py_XDECREF(val); + return ret; } static PyObject * -- 1.7.1
On 02/14/2012 04:19 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 | 43 +++++++++++++++++++++++++++++-------------- 1 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 4e8a97e..594aae6 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 *ret = VIR_PY_NONE;
NAK - this increments the reference count on the None object...
+ PyObject *key = NULL; + PyObject *val = NULL; PyObject *pyobj_conn; virConnectPtr conn; unsigned int flags; @@ -2442,32 +2444,45 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) c_retval = virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, flags); LIBVIRT_END_ALLOW_THREADS; if (c_retval < 0) - return VIR_PY_NONE; + return ret;
if (nparams) { if (VIR_ALLOC_N(stats, nparams) < 0) - return VIR_PY_NONE; + return PyErr_NoMemory();
...but this fails to decrement the reference count, so you've leaked a resource. See my comments on v1.
+ + if (PyDict_SetItem(ret, key, val) < 0) { + Py_DECREF(ret); + goto error;
And here, you have a python exception from PyDict_SetItem...
+ } + + Py_DECREF(key); + Py_DECREF(val); }
VIR_FREE(stats); return ret; + +error: + VIR_FREE(stats); + Py_XDECREF(key); + Py_XDECREF(val); + return ret;
...so here, you _want_ to return NULL, not VIR_PY_NONE. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/14/2012 12:19 PM, 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> ---
--- see end of message for patch of suggested changes ---
python/libvirt-override.c | 43 +++++++++++++++++++++++++++++-------------- 1 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 4e8a97e..594aae6 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;
I realized, that "VIR_PY_NONE" actualy increases the reference count on the "none" object. We should not do it unless we need the reference. Set ret to NULL please.
+ PyObject *ret = VIR_PY_NONE; + PyObject *key = NULL; + PyObject *val = NULL; PyObject *pyobj_conn; virConnectPtr conn; unsigned int flags; @@ -2442,32 +2444,45 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) c_retval = virNodeGetMemoryStats(conn, cellNum, NULL,&nparams, flags); LIBVIRT_END_ALLOW_THREADS; if (c_retval< 0) - return VIR_PY_NONE; + return ret;
Use "goto error;" here for consistency.
if (nparams) { if (VIR_ALLOC_N(stats, nparams)< 0) - return VIR_PY_NONE; + return PyErr_NoMemory();
LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virNodeGetMemoryStats(conn, cellNum, stats,&nparams, flags); LIBVIRT_END_ALLOW_THREADS; - if (c_retval< 0) { - VIR_FREE(stats); - return VIR_PY_NONE; - } - } - if (!(ret = PyDict_New())) { - VIR_FREE(stats); - return VIR_PY_NONE; + if (c_retval< 0) + goto error; } + if (!(ret = PyDict_New())) + goto error; + for (i = 0; i< nparams; i++) { - PyDict_SetItem(ret, - libvirt_constcharPtrWrap(stats[i].field), - libvirt_ulonglongWrap(stats[i].value)); + key = libvirt_constcharPtrWrap(stats[i].field); + val = libvirt_ulonglongWrap(stats[i].value); + + if (!key || !val) + goto error; + + if (PyDict_SetItem(ret, key, val)< 0) { + Py_DECREF(ret);
Move this statement to the error section to free the returned dictionary even in case of other errors.
+ goto error; + } + + Py_DECREF(key); + Py_DECREF(val); }
VIR_FREE(stats); return ret; + +error: + VIR_FREE(stats); + Py_XDECREF(key); + Py_XDECREF(val); + return ret;
You'll need to return VIR_PY_NONE here as the
}
static PyObject *
ACK with those changes. Here is a patch of changes that I suggest: diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 594aae6..0409dd4 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2426,7 +2426,7 @@ libvirt_virNodeGetCPUStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) static PyObject * libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { - PyObject *ret = VIR_PY_NONE; + PyObject *ret = NULL; PyObject *key = NULL; PyObject *val = NULL; PyObject *pyobj_conn; @@ -2444,7 +2444,7 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) c_retval = virNodeGetMemoryStats(conn, cellNum, NULL, &nparams, flags); LIBVIRT_END_ALLOW_THREADS; if (c_retval < 0) - return ret; + goto error; if (nparams) { if (VIR_ALLOC_N(stats, nparams) < 0) @@ -2466,10 +2466,8 @@ libvirt_virNodeGetMemoryStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) if (!key || !val) goto error; - if (PyDict_SetItem(ret, key, val) < 0) { - Py_DECREF(ret); + if (PyDict_SetItem(ret, key, val) < 0) goto error; - } Py_DECREF(key); Py_DECREF(val); @@ -2482,7 +2480,8 @@ error: VIR_FREE(stats); Py_XDECREF(key); Py_XDECREF(val); - return ret; + Py_XDECREF(ret); + return VIR_PY_NONE; } static PyObject *
participants (3)
-
ajia＠redhat.com -
Eric Blake -
Peter Krempa