[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2] python: Avoid memory leaks on libvirt_virNodeGetMemoryStats



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 *


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]