Re: [libvirt] [libvirt-python PATCH 18/23] improve usage of cleanup paths

29 Sep 2015

On Sat, Sep 26, 2015 at 10:14:37AM -0400, John Ferlan wrote:
...
On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
...
This removes several code duplicates and also some unusual code structures.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 libvirt-override.c | 159 +++++++++++++++++++++++------------------------------
 1 file changed, 68 insertions(+), 91 deletions(-)

diff --git a/libvirt-override.c b/libvirt-override.c
index 8afe7d7..205f0dd 100644
--- a/libvirt-override.c
+++ b/libvirt-override.c
@@ -439,13 +439,13 @@ libvirt_virDomainGetSchedulerType(PyObject *self ATTRIBUTE_UNUSED,
         return VIR_PY_NONE;
/* convert to a Python tuple of long objects */
-    if ((info = PyTuple_New(2)) == NULL) {
-        VIR_FREE(c_retval);
-        return NULL;
-    }
+    if ((info = PyTuple_New(2)) == NULL)
+        goto cleanup;
PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(c_retval));
     PyTuple_SetItem(info, 1, libvirt_intWrap((long)nparams));
+
+ cleanup:
     VIR_FREE(c_retval);
     return info;
 }
@@ -2401,7 +2401,6 @@ libvirt_virDomainSnapshotListNames(PyObject *self ATTRIBUTE_UNUSED,
             Py_CLEAR(py_retval);
             goto cleanup;
         }
-        VIR_FREE(names[i]);
     }
cleanup:
@@ -3245,8 +3244,8 @@ libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED,
     LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0) {
-        VIR_FREE(freeMems);
-        return VIR_PY_NONE;
+        py_retval = VIR_PY_NONE;
+        goto cleanup;
     }
if ((py_retval = PyList_New(c_retval)) == NULL)
@@ -3425,24 +3424,19 @@ libvirt_virConnectListStoragePools(PyObject *self ATTRIBUTE_UNUSED,
             return VIR_PY_NONE;
         }
     }
-    py_retval = PyList_New(c_retval);
-    if (py_retval == NULL) {
-        if (names) {
-            for (i = 0; i < c_retval; i++)
-                VIR_FREE(names[i]);
-            VIR_FREE(names);
-        }
-        return NULL;
-    }
+
+    if ((py_retval = PyList_New(c_retval)) == NULL)
+        goto cleanup;
if (names) {
-        for (i = 0; i < c_retval; i++) {
+        for (i = 0; i < c_retval; i++)
             PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
-            VIR_FREE(names[i]);
-        }
-        VIR_FREE(names);
     }
+ cleanup:
+    for (i = 0; i < c_retval; i++)
+        VIR_FREE(names[i]);
+    VIR_FREE(names);
     return py_retval;
 }
@@ -3480,24 +3474,20 @@ libvirt_virConnectListDefinedStoragePools(PyObject *self ATTRIBUTE_UNUSED,
             return VIR_PY_NONE;
         }
     }
-    py_retval = PyList_New(c_retval);
-    if (py_retval == NULL) {
-        if (names) {
-            for (i = 0; i < c_retval; i++)
-                VIR_FREE(names[i]);
-            VIR_FREE(names);
-        }
-        return NULL;
-    }
+
+    if ((py_retval = PyList_New(c_retval)) == NULL)
+        goto cleanup;
if (names) {
-        for (i = 0; i < c_retval; i++) {
+        for (i = 0; i < c_retval; i++)
             PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
-            VIR_FREE(names[i]);
-        }
-        VIR_FREE(names);
     }
+ cleanup:
+    for (i = 0; i < c_retval; i++)
+        VIR_FREE(names[i]);
+    VIR_FREE(names);
+
     return py_retval;
 }
@@ -3579,28 +3569,24 @@ libvirt_virStoragePoolListVolumes(PyObject *self ATTRIBUTE_UNUSED,
         c_retval = virStoragePoolListVolumes(pool, names, c_retval);
         LIBVIRT_END_ALLOW_THREADS;
         if (c_retval < 0) {
-            VIR_FREE(names);
-            return VIR_PY_NONE;
+            py_retval = VIR_PY_NONE;
+            goto cleanup;
We're going to cleanup with c_retval == -1, right... and then doing the
for loop to free names - could be a long time to complete.
I do note that this differs from other prior uses in this patch - that
is other functions will keep the VIR_FREE(names); return VIR_PY_NONE; if
c_retval < 0
At this point it may be worthwhile for you to check *all* such goto's
for c_retval && loops to free memory...
Oh, I didn't realize this, because some functions have 'int i;' and others have
'size_t i'.  I've verified the 'i' declaration only in function with int and
conclude it's ok to use it this way.  This needs to be fixed, I'll send a v2.

Pavel
...
...
}
     }
-    py_retval = PyList_New(c_retval);
-    if (py_retval == NULL) {
-        if (names) {
-            for (i = 0; i < c_retval; i++)
-                VIR_FREE(names[i]);
-            VIR_FREE(names);
-        }
-        return NULL;
-    }
+
+    if ((py_retval = PyList_New(c_retval)) == NULL)
+        goto cleanup;
if (names) {
-        for (i = 0; i < c_retval; i++) {
+        for (i = 0; i < c_retval; i++)
             PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
-            VIR_FREE(names[i]);
-        }
-        VIR_FREE(names);
     }
+ cleanup:
+    for (i = 0; i < c_retval; i++)
+        VIR_FREE(names[i]);
+    VIR_FREE(names);
+
     return py_retval;
 }
@@ -3850,9 +3836,10 @@ libvirt_virNodeListDevices(PyObject *self ATTRIBUTE_UNUSED,
         LIBVIRT_BEGIN_ALLOW_THREADS;
         c_retval = virNodeListDevices(conn, cap, names, c_retval, flags);
         LIBVIRT_END_ALLOW_THREADS;
+
         if (c_retval < 0) {
-            VIR_FREE(names);
-            return VIR_PY_NONE;
+            py_retval = VIR_PY_NONE;
+            goto cleanup;
oh - look at that... here's a similar comment regarding c_retval as above.
...
}
     }
@@ -3947,8 +3934,8 @@ libvirt_virNodeDeviceListCaps(PyObject *self ATTRIBUTE_UNUSED,
         c_retval = virNodeDeviceListCaps(dev, names, c_retval);
         LIBVIRT_END_ALLOW_THREADS;
         if (c_retval < 0) {
-            VIR_FREE(names);
-            return VIR_PY_NONE;
+            py_retval = VIR_PY_NONE;
+            goto cleanup;
again
...
}
     }
@@ -4072,8 +4059,8 @@ libvirt_virConnectListSecrets(PyObject *self ATTRIBUTE_UNUSED,
         c_retval = virConnectListSecrets(conn, uuids, c_retval);
         LIBVIRT_END_ALLOW_THREADS;
         if (c_retval < 0) {
-            VIR_FREE(uuids);
-            return VIR_PY_NONE;
+            py_retval = VIR_PY_NONE;
+            goto cleanup;
again
...
}
     }
@@ -4404,24 +4391,19 @@ libvirt_virConnectListInterfaces(PyObject *self ATTRIBUTE_UNUSED,
             return VIR_PY_NONE;
         }
     }
This one doesn't...
...
-    py_retval = PyList_New(c_retval);
-    if (py_retval == NULL) {
-        if (names) {
-            for (i = 0; i < c_retval; i++)
-                VIR_FREE(names[i]);
-            VIR_FREE(names);
-        }
-        return NULL;
-    }
+
+    if ((py_retval = PyList_New(c_retval)) == NULL)
+        goto cleanup;
if (names) {
-        for (i = 0; i < c_retval; i++) {
+        for (i = 0; i < c_retval; i++)
             PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
-            VIR_FREE(names[i]);
-        }
-        VIR_FREE(names);
     }
+ cleanup:
+    for (i = 0; i < c_retval; i++)
+        VIR_FREE(names[i]);
+    VIR_FREE(names);
     return py_retval;
 }
@@ -4461,24 +4443,19 @@ libvirt_virConnectListDefinedInterfaces(PyObject *self ATTRIBUTE_UNUSED,
             return VIR_PY_NONE;
         }
     }
-    py_retval = PyList_New(c_retval);
-    if (py_retval == NULL) {
-        if (names) {
-            for (i = 0; i < c_retval; i++)
-                VIR_FREE(names[i]);
-            VIR_FREE(names);
-        }
-        return NULL;
-    }
+
+    if ((py_retval = PyList_New(c_retval)) == NULL)
+        goto cleanup;
if (names) {
-        for (i = 0; i < c_retval; i++) {
+        for (i = 0; i < c_retval; i++)
             PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
-            VIR_FREE(names[i]);
-        }
-        VIR_FREE(names);
     }
+ cleanup:
+    for (i = 0; i < c_retval; i++)
+        VIR_FREE(names[i]);
+    VIR_FREE(names);
     return py_retval;
 }
@@ -8459,17 +8436,19 @@ libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED,
/* convert to a Python list */
     if ((py_retval = PyList_New(c_retval)) == NULL)
-        goto cleanup;
+        goto error;
for (i = 0; i < c_retval; i++) {
         virDomainFSInfoPtr fs = fsinfo[i];
         PyObject *info, *alias;
if (fs == NULL)
-            goto cleanup;
+            goto error;
+
         info = PyTuple_New(4);
         if (info == NULL)
-            goto cleanup;
+            goto error;
+
         PyList_SetItem(py_retval, i, info);
PyTuple_SetItem(info, 0, libvirt_constcharPtrWrap(fs->mountpoint));
@@ -8478,27 +8457,25 @@ libvirt_virDomainGetFSInfo(PyObject *self ATTRIBUTE_UNUSED,
alias = PyList_New(0);
         if (alias == NULL)
-            goto cleanup;
+            goto error;
PyTuple_SetItem(info, 3, alias);
for (j = 0; j < fs->ndevAlias; j++)
             if (PyList_Append(alias,
                               libvirt_constcharPtrWrap(fs->devAlias[j])) < 0)
-                goto cleanup;
+                goto error;
     }
Perhaps this one was pre-existing, but if c_retval < 0, this isn't going
to go well... or it won't be quick!
John
...
+ cleanup:
     for (i = 0; i < c_retval; i++)
         virDomainFSInfoFree(fsinfo[i]);
     VIR_FREE(fsinfo);
     return py_retval;
- cleanup:
-    for (i = 0; i < c_retval; i++)
-        virDomainFSInfoFree(fsinfo[i]);
-    VIR_FREE(fsinfo);
-    Py_XDECREF(py_retval);
-    return NULL;
+ error:
+    Py_CLEAR(py_retval);
+    goto cleanup;
 }
#endif /* LIBVIR_CHECK_VERSION(1, 2, 11) */


    