Thanks for the review, Rich. Seeing that messed-up Subject: line, I realized that I sent a patch produced by "git-format-patch", forgetting that the ancient version of mailman behind this list would remove important parts of it. For the record, here's what I really sent: (with the "From " line escaped) From: Jim Meyering <jim(a)meyering.net&gt; To: "Daniel P. Berrange" <berrange(a)redhat.com&gt; Cc: Libvirt <libvir-list(a)redhat.com&gt; Subject: Re: [Libvir] handle PyTuple_New's malloc failure In-Reply-To: <20080117155200.GC21549(a)redhat.com&gt; (Daniel P. Berrange's message of "Thu, 17 Jan 2008 15:52:00 +0000") References: <87zlv4larm.fsf(a)rho.meyering.net&gt; <20080117155200.GC21549(a)redhat.com&gt; Date: Thu, 17 Jan 2008 19:59:07 +0100 Message-ID: <87y7aojn2c.fsf(a)rho.meyering.net&gt; "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: Good idea. That makes the patch a lot bigger, but makes the resulting code more readable, too. Makes sense. And with VIR_PY_NONE, the duplication isn't a problem. New patch below. Me too. Here's the combined patch. Considering the number of uses of VIR_PY_NONE this introduces, I'll have to do the right thing and split it into two: one that adds VIR_PY_NONE, and another that fixes the NULL-deref bugs. two-statement sequence. Handle PyTuple_New's malloc failure. Use VIR_PY_NONE. Signed-off-by: Jim Meyering <meyering(a)redhat.com&gt; --- python/libvir.c | 177 +++++++++++++++++++++---------------------------------- 1 files changed, 68 insertions(+), 109 deletions(-) diff --git a/python/libvir.c b/python/libvir.c index 3b41dc1..dbf148e 100644 --- a/python/libvir.c +++ b/python/libvir.c @@ -31,6 +31,11 @@ PyObject * libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED, PyObject PyObject * libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args); PyObject * libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED, PyObject *args); +/* The two-statement sequence "Py_INCREF(Py_None); return Py_None;" + is so common that we encapsulate it here. Now, each use is simply + return VIR_PY_NONE; */ +#define VIR_PY_NONE (Py_INCREF (Py_None), Py_None) + /************************************************************************ * * * Statistics * @@ -48,17 +53,16 @@ libvirt_virDomainBlockStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { if (!PyArg_ParseTuple(args, (char *)"Oz:virDomainBlockStats", &pyobj_domain,&path)) - return(NULL); + return(NULL); domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); c_retval = virDomainBlockStats(domain, path, &stats, sizeof(stats)); - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; - /* convert to a Python tupple of long objects */ - info = PyTuple_New(5); + /* convert to a Python tuple of long objects */ + if ((info = PyTuple_New(5)) == NULL) + return VIR_PY_NONE; PyTuple_SetItem(info, 0, PyLong_FromLongLong(stats.rd_req)); PyTuple_SetItem(info, 1, PyLong_FromLongLong(stats.rd_bytes)); PyTuple_SetItem(info, 2, PyLong_FromLongLong(stats.wr_req)); @@ -82,13 +86,12 @@ libvirt_virDomainInterfaceStats(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); c_retval = virDomainInterfaceStats(domain, path, &stats, sizeof(stats)); - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; - /* convert to a Python tupple of long objects */ - info = PyTuple_New(8); + /* convert to a Python tuple of long objects */ + if ((info = PyTuple_New(8)) == NULL) + return VIR_PY_NONE; PyTuple_SetItem(info, 0, PyLong_FromLongLong(stats.rx_bytes)); PyTuple_SetItem(info, 1, PyLong_FromLongLong(stats.rx_packets)); PyTuple_SetItem(info, 2, PyLong_FromLongLong(stats.rx_errs)); @@ -114,12 +117,11 @@ libvirt_virGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject *args ATTRIBUT virError err; PyObject *info; - if (virCopyLastError(&err) <= 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (virCopyLastError(&err) <= 0) + return VIR_PY_NONE; - info = PyTuple_New(9); + if ((info = PyTuple_New(9)) == NULL) + return VIR_PY_NONE; PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code)); PyTuple_SetItem(info, 1, PyInt_FromLong((long) err.domain)); PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err.message)); @@ -145,12 +147,11 @@ libvirt_virConnGetLastError(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) return(NULL); conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); - if (virConnCopyLastError(conn, &err) <= 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (virConnCopyLastError(conn, &err) <= 0) + return VIR_PY_NONE; - info = PyTuple_New(9); + if ((info = PyTuple_New(9)) == NULL) + return VIR_PY_NONE; PyTuple_SetItem(info, 0, PyInt_FromLong((long) err.code)); PyTuple_SetItem(info, 1, PyInt_FromLong((long) err.domain)); PyTuple_SetItem(info, 2, libvirt_constcharPtrWrap(err.message)); @@ -348,10 +349,8 @@ libvirt_virConnectOpenAuth(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { if (auth.ncredtype) { int i; auth.credtype = malloc(sizeof(*auth.credtype) * auth.ncredtype); - if (auth.credtype == NULL) { - Py_INCREF(Py_None); - return (Py_None); - } + if (auth.credtype == NULL) + return VIR_PY_NONE; for (i = 0 ; i < auth.ncredtype ; i++) { PyObject *val; val = PyList_GetItem(pycredtype, i); @@ -395,10 +394,8 @@ libvirt_virGetVersion (PyObject *self ATTRIBUTE_UNUSED, PyObject *args) LIBVIRT_END_ALLOW_THREADS; - if (c_retval == -1) { - Py_INCREF(Py_None); - return (Py_None); - } + if (c_retval == -1) + return VIR_PY_NONE; if (type == NULL) return PyInt_FromLong (libVer); @@ -458,10 +455,8 @@ libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED, LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virConnectListDomains(conn, &ids[0], 500); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = PyList_New(c_retval); for (i = 0;i < c_retval;i++) { PyList_SetItem(py_retval, i, libvirt_intWrap(ids[i])); @@ -484,22 +479,17 @@ libvirt_virConnectListDefinedDomains(PyObject *self ATTRIBUTE_UNUSED, conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); c_retval = virConnectNumOfDefinedDomains(conn); - if (c_retval < 0) { - Py_INCREF(Py_None); - return (Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; if (c_retval) { names = malloc(sizeof(*names) * c_retval); - if (!names) { - Py_INCREF(Py_None); - return (Py_None); - } + if (!names) + return VIR_PY_NONE; c_retval = virConnectListDefinedDomains(conn, names, c_retval); if (c_retval < 0) { free(names); - Py_INCREF(Py_None); - return(Py_None); + return VIR_PY_NONE; } } py_retval = PyList_New(c_retval); @@ -530,10 +520,8 @@ libvirt_virDomainGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virDomainGetInfo(domain, &info); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = PyList_New(5); PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.state)); PyList_SetItem(py_retval, 1, libvirt_ulongWrap(info.maxMem)); @@ -559,10 +547,8 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virNodeGetInfo(conn, &info); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = PyList_New(8); PyList_SetItem(py_retval, 0, libvirt_constcharPtrWrap(&info.model[0])); PyList_SetItem(py_retval, 1, libvirt_longWrap((long) info.memory >> 10)); @@ -587,18 +573,14 @@ libvirt_virDomainGetUUID(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { return(NULL); domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); - if (domain == NULL) { - Py_INCREF(Py_None); - return(Py_None); - } + if (domain == NULL) + return VIR_PY_NONE; LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virDomainGetUUID(domain, &uuid[0]); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = PyString_FromStringAndSize((char *) &uuid[0], VIR_UUID_BUFLEN); return(py_retval); @@ -617,10 +599,8 @@ libvirt_virDomainLookupByUUID(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { return(NULL); conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); - if ((uuid == NULL) || (len != VIR_UUID_BUFLEN)) { - Py_INCREF(Py_None); - return(Py_None); - } + if ((uuid == NULL) || (len != VIR_UUID_BUFLEN)) + return VIR_PY_NONE; LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virDomainLookupByUUID(conn, uuid); @@ -664,22 +644,17 @@ libvirt_virConnectListNetworks(PyObject *self ATTRIBUTE_UNUSED, conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); c_retval = virConnectNumOfNetworks(conn); - if (c_retval < 0) { - Py_INCREF(Py_None); - return (Py_None); - } - + if (c_retval < 0) + return VIR_PY_NONE; + if (c_retval) { names = malloc(sizeof(*names) * c_retval); - if (!names) { - Py_INCREF(Py_None); - return (Py_None); - } + if (!names) + return VIR_PY_NONE; c_retval = virConnectListNetworks(conn, names, c_retval); if (c_retval < 0) { free(names); - Py_INCREF(Py_None); - return(Py_None); + return VIR_PY_NONE; } } py_retval = PyList_New(c_retval); @@ -711,22 +686,17 @@ libvirt_virConnectListDefinedNetworks(PyObject *self ATTRIBUTE_UNUSED, conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); c_retval = virConnectNumOfDefinedNetworks(conn); - if (c_retval < 0) { - Py_INCREF(Py_None); - return (Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; if (c_retval) { names = malloc(sizeof(*names) * c_retval); - if (!names) { - Py_INCREF(Py_None); - return (Py_None); - } + if (!names) + return VIR_PY_NONE; c_retval = virConnectListDefinedNetworks(conn, names, c_retval); if (c_retval < 0) { free(names); - Py_INCREF(Py_None); - return(Py_None); + return VIR_PY_NONE; } } py_retval = PyList_New(c_retval); @@ -755,18 +725,14 @@ libvirt_virNetworkGetUUID(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { return(NULL); domain = (virNetworkPtr) PyvirNetwork_Get(pyobj_domain); - if (domain == NULL) { - Py_INCREF(Py_None); - return(Py_None); - } + if (domain == NULL) + return VIR_PY_NONE; LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virNetworkGetUUID(domain, &uuid[0]); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return(Py_None); - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = PyString_FromStringAndSize((char *) &uuid[0], VIR_UUID_BUFLEN); return(py_retval); @@ -785,10 +751,8 @@ libvirt_virNetworkLookupByUUID(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) return(NULL); conn = (virConnectPtr) PyvirConnect_Get(pyobj_conn); - if ((uuid == NULL) || (len != VIR_UUID_BUFLEN)) { - Py_INCREF(Py_None); - return(Py_None); - } + if ((uuid == NULL) || (len != VIR_UUID_BUFLEN)) + return VIR_PY_NONE; LIBVIRT_BEGIN_ALLOW_THREADS; c_retval = virNetworkLookupByUUID(conn, uuid); @@ -814,10 +778,8 @@ libvirt_virDomainGetAutostart(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { c_retval = virDomainGetAutostart(domain, &autostart); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return Py_None; - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = libvirt_intWrap(autostart); return(py_retval); } @@ -839,10 +801,8 @@ libvirt_virNetworkGetAutostart(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) c_retval = virNetworkGetAutostart(network, &autostart); LIBVIRT_END_ALLOW_THREADS; - if (c_retval < 0) { - Py_INCREF(Py_None); - return Py_None; - } + if (c_retval < 0) + return VIR_PY_NONE; py_retval = libvirt_intWrap(autostart); return(py_retval); } @@ -873,10 +833,9 @@ PyObject * libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED, LIBVIRT_END_ALLOW_THREADS; if (c_retval < 0) { - free(freeMems); + free(freeMems); error: - Py_INCREF(Py_None); - return Py_None; + return VIR_PY_NONE; } py_retval = PyList_New(c_retval); for (i = 0;i < c_retval;i++) { -- 1.5.4.rc3.14.g44397