Re: [libvirt] [libvirt-python PATCH 15/23] all Py*_New function has to be checked for return value

$subj: Must check return value for all Py*_New functions

On 09/24/2015 10:01 AM, Pavel Hrdina wrote: > If the function fails, we need to cleanup memory and return NULL. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > libvirt-lxc-override.c | 10 +- > libvirt-override.c | 269 +++++++++++++++++++++++++++++++++---------------- > 2 files changed, 190 insertions(+), 89 deletions(-) > General comment... 4 patches ago we were returning PyErr_NoMemory on allocation failures - that would seem to be the case for these too, right? Or is this "just different" enough that it would be necessary?

> diff --git a/libvirt-lxc-override.c b/libvirt-lxc-override.c > index 20d1cf4..b0550c7 100644 > --- a/libvirt-lxc-override.c > +++ b/libvirt-lxc-override.c > @@ -79,7 +79,9 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED, > if (c_retval < 0) > return VIR_PY_NONE; > > - py_retval = PyList_New(0); > + if ((py_retval = PyList_New(0)) == NULL) > + goto error; > + > for (i = 0; i < c_retval; i++) { > PyObject *item = NULL; > > @@ -91,6 +93,8 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED, > goto error; > } > } > + > + cleanup: > VIR_FREE(fdlist); > return py_retval; > > @@ -98,8 +102,8 @@ libvirt_lxc_virDomainLxcOpenNamespace(PyObject *self ATTRIBUTE_UNUSED, > for (i = 0; i < c_retval; i++) { > VIR_FORCE_CLOSE(fdlist[i]); > } > - VIR_FREE(fdlist); > - return NULL; > + Py_CLEAR(py_retval); > + goto cleanup; > } > /************************************************************************ > * * > diff --git a/libvirt-override.c b/libvirt-override.c > index 0df6844..92c31b8 100644 > --- a/libvirt-override.c > +++ b/libvirt-override.c > @@ -1859,7 +1859,7 @@ static void > libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx, > virErrorPtr err) > { > - PyObject *list, *info; > + PyObject *list = NULL, *info = NULL; > PyObject *result; > > DEBUG("libvirt_virErrorFuncHandler(%p, %s, ...) called\n", ctx, > @@ -1874,8 +1874,12 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx, > (libvirt_virPythonErrorFuncHandler == Py_None)) { > virDefaultErrorFunc(err); > } else { > - list = PyTuple_New(2); > - info = PyTuple_New(9); > + if ((list = PyTuple_New(2)) == NULL) > + goto cleanup; > + > + if ((info = PyTuple_New(9)) == NULL) > + goto cleanup; > + > PyTuple_SetItem(list, 0, libvirt_virPythonErrorFuncCtxt); > PyTuple_SetItem(list, 1, info); > Py_XINCREF(libvirt_virPythonErrorFuncCtxt); > @@ -1894,6 +1898,10 @@ libvirt_virErrorFuncHandler(ATTRIBUTE_UNUSED void *ctx, > Py_XDECREF(result); > } > > + cleanup: > + Py_XDECREF(list); > + Py_XDECREF(info); > + > LIBVIRT_RELEASE_THREAD_STATE; > } > > @@ -1938,12 +1946,12 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred, > unsigned int ncred, > void *cbdata) > { > - PyObject *list; > + PyObject *list = NULL; > PyObject *pycred; > PyObject *pyauth = (PyObject *)cbdata; > PyObject *pycbdata; > PyObject *pycb; > - PyObject *pyret; > + PyObject *pyret = NULL; > int ret = -1; > size_t i; > > @@ -1952,11 +1960,17 @@ virConnectCredCallbackWrapper(virConnectCredentialPtr cred, > pycb = PyList_GetItem(pyauth, 1); > pycbdata = PyList_GetItem(pyauth, 2); > > - list = PyTuple_New(2); > - pycred = PyTuple_New(ncred); > + if((list = PyTuple_New(2)) == NULL) if ((

> + goto cleanup; > + > + if ((pycred = PyTuple_New(ncred)) == NULL) > + goto cleanup; > + > for (i = 0; i < ncred; i++) { > - PyObject *pycreditem; > - pycreditem = PyList_New(5); > + PyObject *pycreditem = PyList_New(5); > + if (pycreditem == NULL) > + goto cleanup; > + nit: could all be one line (any typos are mine ;-)) if ((pycreditem = PyList_New(5)) == NULL) Nothing jumped out at me in the rest - although I admit my eyes & brain are starting to tire ;-)

John > PyTuple_SetItem(pycred, i, pycreditem); > PyList_SetItem(pycreditem, 0, libvirt_intWrap((long) cred[i].type)); > PyList_SetItem(pycreditem, 1, libvirt_constcharPtrWrap(cred[i].prompt));