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@xxxxxxxxxxxx>
To: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
Cc: Libvirt <libvir-list@xxxxxxxxxx>
Subject: Re: handle PyTuple_New's malloc failure
In-Reply-To: <20080117155200.GC21549@xxxxxxxxxx> (Daniel P. Berrange's message
of "Thu, 17 Jan 2008 15:52:00 +0000")
References: <87zlv4larm.fsf@xxxxxxxxxxxxxxxx>
<20080117155200.GC21549@xxxxxxxxxx>
Date: Thu, 17 Jan 2008 19:59:07 +0100
Message-ID: <87y7aojn2c.fsf@xxxxxxxxxxxxxxxx>
"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Thu, Jan 17, 2008 at 04:41:49PM +0100, Jim Meyering wrote:
>> python/libvir.c calls PyTuple_New, but doesn't handle the possibility
>> of a NULL return value. Subsequent use of the result pointer in e.g.,
>> PyTuple_SetItem, would dereference it.
>>
>> This fixes most of them, but leaves the ones in
>> virConnectCredCallbackWrapper untouched. Fixing them
>> properly is not as easy, and IMHO will require actual testing.
>>
>> In this patch, I combined each new test like this
>>
>> ((info = PyTuple_New(8)) == NULL)
>>
>> with the preceding "if (c_retval < 0)", since the bodies would be the same.
>>
>> Duplicating that 2-line body might look more readable,
>> depending on which side of the bed you get up on...
>> I can go either way.
> Might be useful to have a macro to combine the Py_INCREF and return
> (Py_None) in one convenient blob. return VIR_PY_NONE();
Good idea. That makes the patch a lot bigger, but makes the
resulting code more readable, too.
> I think it'd be nicer to keep the PyTuple_new bit separate as it is
> now, because in some APIs there can be other code between the existing
> c_retval < 0 check, and the point at which we create the Tuple.
>
> eg, the libvirt_virDomainGetSchedulerParameters method in the
> patch I'm attaching which implements a number of missing APIs
> NB, this patch is not tested yet, so not intended to be applied
Makes sense. And with VIR_PY_NONE, the duplication isn't a problem.
New patch below.
> I curious as to why we ever return(NULL) here rather than Py_None. I'm
> not sure of the python runtime / C binding contract - but returning an
> actual NULL seems odd.
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.
> From 55bafd1bdaf6dde1bd019397f569d397fea366a6 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@xxxxxxxxxx>
> Date: Thu, 17 Jan 2008 16:40:17 +0100
> Subject: [PATCH] python/libvir.c (VIR_PY_NONE): New macro, to encapsulate a common
two-statement sequence.
Handle PyTuple_New's malloc failure.
Use VIR_PY_NONE.
Signed-off-by: Jim Meyering <meyering@xxxxxxxxxx>
---
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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list