[libvirt] [libvirt-python PATCH 18/23] improve usage of cleanup paths
John Ferlan
jferlan at redhat.com
Sat Sep 26 14:14:37 UTC 2015
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 at 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...
> }
> }
> - 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) */
>
More information about the libvir-list
mailing list