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

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(a)redhat.com&gt; > --- > 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...

> } > } > - 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) */ >