8:44 a.m.
Giuseppe Scrivano (2):
python: prefer PyList_SET_ITEM to PyList_SetItem
python: add error checking for calls to PyList_New
python/libvirt-override.c | 285 ++++++++++++++++++++++++++--------------------
1 file changed, 160 insertions(+), 125 deletions(-)
--
1.8.3.1
8:44 a.m.
New subject: [libvirt] [PATCH 1/2] python: prefer PyList_SET_ITEM to PyList_SetItem
The PyList_SET_ITEM macro, differently from PyList_SetItem, doesn't do
any error checking and overwrites anything that was previously stored
in the list at the chosen destination position.
PyList_SET_ITEM is usually faster than PyList_SetItem, and since it is
used only to fill freshly created lists, it is safe to be used here.
Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
---
python/libvirt-override.c | 197 +++++++++++++++++++++-------------------------
1 file changed, 90 insertions(+), 107 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 2e58bf9..83bca94 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -1588,8 +1588,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
PyTuple_SetItem(info, 3, item) < 0)
goto itemError;
- if (PyList_SetItem(pycpuinfo, i, info) < 0)
- goto itemError;
+ PyList_SET_ITEM(pycpuinfo, i, info);
continue;
itemError:
@@ -1611,10 +1610,7 @@ libvirt_virDomainGetVcpus(PyObject *self ATTRIBUTE_UNUSED,
goto cleanup;
}
}
- if (PyList_SetItem(pycpumap, i, info) < 0) {
- Py_DECREF(info);
- goto cleanup;
- }
+ PyList_SET_ITEM(pycpumap, i, info);
}
if (PyTuple_SetItem(pyretval, 0, pycpuinfo) < 0 ||
PyTuple_SetItem(pyretval, 1, pycpumap) < 0)
@@ -1812,7 +1808,7 @@ libvirt_virDomainGetVcpuPinInfo(PyObject *self ATTRIBUTE_UNUSED,
PyTuple_SetItem(mapinfo, pcpu,
PyBool_FromLong(VIR_CPU_USABLE(cpumaps, cpumaplen, vcpu,
pcpu)));
}
- PyList_SetItem(pycpumaps, vcpu, mapinfo);
+ PyList_SET_ITEM(pycpumaps, vcpu, mapinfo);
}
VIR_FREE(cpumaps);
@@ -2115,21 +2111,21 @@ static int virConnectCredCallbackWrapper(virConnectCredentialPtr
cred,
pycreditem = PyList_New(5);
Py_INCREF(Py_None);
PyTuple_SetItem(pycred, i, pycreditem);
- PyList_SetItem(pycreditem, 0, PyInt_FromLong((long) cred[i].type));
- PyList_SetItem(pycreditem, 1, PyString_FromString(cred[i].prompt));
+ PyList_SET_ITEM(pycreditem, 0, PyInt_FromLong((long) cred[i].type));
+ PyList_SET_ITEM(pycreditem, 1, PyString_FromString(cred[i].prompt));
if (cred[i].challenge) {
- PyList_SetItem(pycreditem, 2, PyString_FromString(cred[i].challenge));
+ PyList_SET_ITEM(pycreditem, 2, PyString_FromString(cred[i].challenge));
} else {
Py_INCREF(Py_None);
- PyList_SetItem(pycreditem, 2, Py_None);
+ PyList_SET_ITEM(pycreditem, 2, Py_None);
}
if (cred[i].defresult) {
- PyList_SetItem(pycreditem, 3, PyString_FromString(cred[i].defresult));
+ PyList_SET_ITEM(pycreditem, 3, PyString_FromString(cred[i].defresult));
} else {
Py_INCREF(Py_None);
- PyList_SetItem(pycreditem, 3, Py_None);
+ PyList_SET_ITEM(pycreditem, 3, Py_None);
}
- PyList_SetItem(pycreditem, 4, Py_None);
+ PyList_SET_ITEM(pycreditem, 4, Py_None);
}
PyTuple_SetItem(list, 0, pycred);
@@ -2390,7 +2386,7 @@ libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED,
if (ids) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_intWrap(ids[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_intWrap(ids[i]));
}
VIR_FREE(ids);
}
@@ -2426,13 +2422,12 @@ libvirt_virConnectListAllDomains(PyObject *self ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virDomainPtrWrap(doms[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virDomainPtrWrap(doms[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
doms[i] = NULL;
}
@@ -2481,7 +2476,7 @@ libvirt_virConnectListDefinedDomains(PyObject *self
ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -2530,13 +2525,12 @@ libvirt_virDomainSnapshotListNames(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL ||
- PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
- Py_XDECREF(pyobj_snap);
+ if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, pyobj_snap);
VIR_FREE(names[i]);
}
@@ -2575,13 +2569,12 @@ libvirt_virDomainListAllSnapshots(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL ||
- PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
- Py_XDECREF(pyobj_snap);
+ if (!(pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, pyobj_snap);
snaps[i] = NULL;
}
@@ -2631,13 +2624,12 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
py_retval = PyList_New(c_retval);
for (i = 0; i < c_retval; i++) {
- if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL ||
- PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
- Py_XDECREF(pyobj_snap);
+ if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, pyobj_snap);
VIR_FREE(names[i]);
}
@@ -2676,13 +2668,12 @@ libvirt_virDomainSnapshotListAllChildren(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL ||
- PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
- Py_XDECREF(pyobj_snap);
+ if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, pyobj_snap);
snaps[i] = NULL;
}
@@ -2734,11 +2725,11 @@ libvirt_virDomainGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
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));
- PyList_SetItem(py_retval, 2, libvirt_ulongWrap(info.memory));
- PyList_SetItem(py_retval, 3, libvirt_intWrap((int) info.nrVirtCpu));
- PyList_SetItem(py_retval, 4,
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.state));
+ PyList_SET_ITEM(py_retval, 1, libvirt_ulongWrap(info.maxMem));
+ PyList_SET_ITEM(py_retval, 2, libvirt_ulongWrap(info.memory));
+ PyList_SET_ITEM(py_retval, 3, libvirt_intWrap((int) info.nrVirtCpu));
+ PyList_SET_ITEM(py_retval, 4,
libvirt_longlongWrap((unsigned long long) info.cpuTime));
return py_retval;
}
@@ -2767,8 +2758,8 @@ libvirt_virDomainGetState(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args)
return VIR_PY_NONE;
py_retval = PyList_New(2);
- PyList_SetItem(py_retval, 0, libvirt_intWrap(state));
- PyList_SetItem(py_retval, 1, libvirt_intWrap(reason));
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap(state));
+ PyList_SET_ITEM(py_retval, 1, libvirt_intWrap(reason));
return py_retval;
}
@@ -2792,9 +2783,9 @@ libvirt_virDomainGetControlInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
if (c_retval < 0)
return VIR_PY_NONE;
py_retval = PyList_New(3);
- PyList_SetItem(py_retval, 0, libvirt_intWrap(info.state));
- PyList_SetItem(py_retval, 1, libvirt_intWrap(info.details));
- PyList_SetItem(py_retval, 2, libvirt_longlongWrap(info.stateTime));
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap(info.state));
+ PyList_SET_ITEM(py_retval, 1, libvirt_intWrap(info.details));
+ PyList_SET_ITEM(py_retval, 2, libvirt_longlongWrap(info.stateTime));
return py_retval;
}
@@ -2818,9 +2809,9 @@ libvirt_virDomainGetBlockInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
if (c_retval < 0)
return VIR_PY_NONE;
py_retval = PyList_New(3);
- PyList_SetItem(py_retval, 0, libvirt_ulonglongWrap(info.capacity));
- PyList_SetItem(py_retval, 1, libvirt_ulonglongWrap(info.allocation));
- PyList_SetItem(py_retval, 2, libvirt_ulonglongWrap(info.physical));
+ PyList_SET_ITEM(py_retval, 0, libvirt_ulonglongWrap(info.capacity));
+ PyList_SET_ITEM(py_retval, 1, libvirt_ulonglongWrap(info.allocation));
+ PyList_SET_ITEM(py_retval, 2, libvirt_ulonglongWrap(info.physical));
return py_retval;
}
@@ -2842,14 +2833,14 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
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));
- PyList_SetItem(py_retval, 2, libvirt_intWrap((int) info.cpus));
- PyList_SetItem(py_retval, 3, libvirt_intWrap((int) info.mhz));
- PyList_SetItem(py_retval, 4, libvirt_intWrap((int) info.nodes));
- PyList_SetItem(py_retval, 5, libvirt_intWrap((int) info.sockets));
- PyList_SetItem(py_retval, 6, libvirt_intWrap((int) info.cores));
- PyList_SetItem(py_retval, 7, libvirt_intWrap((int) info.threads));
+ PyList_SET_ITEM(py_retval, 0, libvirt_constcharPtrWrap(&info.model[0]));
+ PyList_SET_ITEM(py_retval, 1, libvirt_longWrap((long) info.memory >> 10));
+ PyList_SET_ITEM(py_retval, 2, libvirt_intWrap((int) info.cpus));
+ PyList_SET_ITEM(py_retval, 3, libvirt_intWrap((int) info.mhz));
+ PyList_SET_ITEM(py_retval, 4, libvirt_intWrap((int) info.nodes));
+ PyList_SET_ITEM(py_retval, 5, libvirt_intWrap((int) info.sockets));
+ PyList_SET_ITEM(py_retval, 6, libvirt_intWrap((int) info.cores));
+ PyList_SET_ITEM(py_retval, 7, libvirt_intWrap((int) info.threads));
return py_retval;
}
@@ -2965,7 +2956,7 @@ libvirt_virConnectListNetworks(PyObject *self ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -3011,7 +3002,7 @@ libvirt_virConnectListDefinedNetworks(PyObject *self
ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -3048,13 +3039,12 @@ libvirt_virConnectListAllNetworks(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virNetworkPtrWrap(nets[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virNetworkPtrWrap(nets[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
nets[i] = NULL;
}
@@ -3219,7 +3209,7 @@ libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED,
PyObject *arg
}
py_retval = PyList_New(c_retval);
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i,
+ PyList_SET_ITEM(py_retval, i,
libvirt_longlongWrap((long long) freeMems[i]));
}
VIR_FREE(freeMems);
@@ -3398,7 +3388,7 @@ libvirt_virConnectListStoragePools(PyObject *self ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -3452,7 +3442,7 @@ libvirt_virConnectListDefinedStoragePools(PyObject *self
ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -3489,13 +3479,12 @@ libvirt_virConnectListAllStoragePools(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virStoragePoolPtrWrap(pools[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virStoragePoolPtrWrap(pools[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
pools[i] = NULL;
}
@@ -3552,7 +3541,7 @@ libvirt_virStoragePoolListVolumes(PyObject *self ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -3590,13 +3579,12 @@ libvirt_virStoragePoolListAllVolumes(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virStorageVolPtrWrap(vols[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virStorageVolPtrWrap(vols[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
vols[i] = NULL;
}
@@ -3654,12 +3642,12 @@ libvirt_virStoragePoolGetInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
if ((py_retval = PyList_New(4)) == NULL)
return VIR_PY_NONE;
- PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.state));
- PyList_SetItem(py_retval, 1,
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.state));
+ PyList_SET_ITEM(py_retval, 1,
libvirt_longlongWrap((unsigned long long) info.capacity));
- PyList_SetItem(py_retval, 2,
+ PyList_SET_ITEM(py_retval, 2,
libvirt_longlongWrap((unsigned long long) info.allocation));
- PyList_SetItem(py_retval, 3,
+ PyList_SET_ITEM(py_retval, 3,
libvirt_longlongWrap((unsigned long long) info.available));
return py_retval;
}
@@ -3685,10 +3673,10 @@ libvirt_virStorageVolGetInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
if ((py_retval = PyList_New(3)) == NULL)
return VIR_PY_NONE;
- PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.type));
- PyList_SetItem(py_retval, 1,
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.type));
+ PyList_SET_ITEM(py_retval, 1,
libvirt_longlongWrap((unsigned long long) info.capacity));
- PyList_SetItem(py_retval, 2,
+ PyList_SET_ITEM(py_retval, 2,
libvirt_longlongWrap((unsigned long long) info.allocation));
return py_retval;
}
@@ -3806,7 +3794,7 @@ libvirt_virNodeListDevices(PyObject *self ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -3843,13 +3831,12 @@ libvirt_virConnectListAllNodeDevices(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virNodeDevicePtrWrap(devices[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virNodeDevicePtrWrap(devices[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
devices[i] = NULL;
}
@@ -3897,7 +3884,7 @@ libvirt_virNodeDeviceListCaps(PyObject *self ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -4017,7 +4004,7 @@ libvirt_virConnectListSecrets(PyObject *self ATTRIBUTE_UNUSED,
if (uuids) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(uuids[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(uuids[i]));
VIR_FREE(uuids[i]);
}
VIR_FREE(uuids);
@@ -4054,13 +4041,12 @@ libvirt_virConnectListAllSecrets(PyObject *self ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virSecretPtrWrap(secrets[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virSecretPtrWrap(secrets[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
secrets[i] = NULL;
}
@@ -4237,7 +4223,7 @@ libvirt_virConnectListNWFilters(PyObject *self ATTRIBUTE_UNUSED,
if (uuids) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(uuids[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(uuids[i]));
VIR_FREE(uuids[i]);
}
VIR_FREE(uuids);
@@ -4274,13 +4260,12 @@ libvirt_virConnectListAllNWFilters(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virNWFilterPtrWrap(filters[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virNWFilterPtrWrap(filters[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
filters[i] = NULL;
}
@@ -4337,7 +4322,7 @@ libvirt_virConnectListInterfaces(PyObject *self ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -4392,7 +4377,7 @@ libvirt_virConnectListDefinedInterfaces(PyObject *self
ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
@@ -4430,13 +4415,12 @@ libvirt_virConnectListAllInterfaces(PyObject *self
ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if (!(tmp = libvirt_virInterfacePtrWrap(ifaces[i])) ||
- PyList_SetItem(py_retval, i, tmp) < 0) {
- Py_XDECREF(tmp);
+ if (!(tmp = libvirt_virInterfacePtrWrap(ifaces[i]))) {
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, tmp);
/* python steals the pointer */
ifaces[i] = NULL;
}
@@ -4519,18 +4503,18 @@ libvirt_virDomainGetJobInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
if (c_retval < 0)
return VIR_PY_NONE;
py_retval = PyList_New(12);
- PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.type));
- PyList_SetItem(py_retval, 1, libvirt_ulonglongWrap(info.timeElapsed));
- PyList_SetItem(py_retval, 2, libvirt_ulonglongWrap(info.timeRemaining));
- PyList_SetItem(py_retval, 3, libvirt_ulonglongWrap(info.dataTotal));
- PyList_SetItem(py_retval, 4, libvirt_ulonglongWrap(info.dataProcessed));
- PyList_SetItem(py_retval, 5, libvirt_ulonglongWrap(info.dataRemaining));
- PyList_SetItem(py_retval, 6, libvirt_ulonglongWrap(info.memTotal));
- PyList_SetItem(py_retval, 7, libvirt_ulonglongWrap(info.memProcessed));
- PyList_SetItem(py_retval, 8, libvirt_ulonglongWrap(info.memRemaining));
- PyList_SetItem(py_retval, 9, libvirt_ulonglongWrap(info.fileTotal));
- PyList_SetItem(py_retval, 10, libvirt_ulonglongWrap(info.fileProcessed));
- PyList_SetItem(py_retval, 11, libvirt_ulonglongWrap(info.fileRemaining));
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.type));
+ PyList_SET_ITEM(py_retval, 1, libvirt_ulonglongWrap(info.timeElapsed));
+ PyList_SET_ITEM(py_retval, 2, libvirt_ulonglongWrap(info.timeRemaining));
+ PyList_SET_ITEM(py_retval, 3, libvirt_ulonglongWrap(info.dataTotal));
+ PyList_SET_ITEM(py_retval, 4, libvirt_ulonglongWrap(info.dataProcessed));
+ PyList_SET_ITEM(py_retval, 5, libvirt_ulonglongWrap(info.dataRemaining));
+ PyList_SET_ITEM(py_retval, 6, libvirt_ulonglongWrap(info.memTotal));
+ PyList_SET_ITEM(py_retval, 7, libvirt_ulonglongWrap(info.memProcessed));
+ PyList_SET_ITEM(py_retval, 8, libvirt_ulonglongWrap(info.memRemaining));
+ PyList_SET_ITEM(py_retval, 9, libvirt_ulonglongWrap(info.fileTotal));
+ PyList_SET_ITEM(py_retval, 10, libvirt_ulonglongWrap(info.fileProcessed));
+ PyList_SET_ITEM(py_retval, 11, libvirt_ulonglongWrap(info.fileRemaining));
return py_retval;
}
@@ -5916,7 +5900,7 @@ libvirt_virConnectDomainEventGraphicsCallback(virConnectPtr conn
ATTRIBUTE_UNUSE
PyTuple_SetItem(pair, 0,
libvirt_constcharPtrWrap(subject->identities[i].type));
PyTuple_SetItem(pair, 1,
libvirt_constcharPtrWrap(subject->identities[i].name));
- PyList_SetItem(pyobj_subject, i, pair);
+ PyList_SET_ITEM(pyobj_subject, i, pair);
}
/* Call the Callback Dispatcher */
@@ -7101,8 +7085,7 @@ libvirt_virNodeGetCPUMap(PyObject *self ATTRIBUTE_UNUSED,
for (i = 0; i < i_retval; i++) {
if ((pyused = PyBool_FromLong(VIR_CPU_USED(cpumap, i))) == NULL)
goto error;
- if (PyList_SetItem(pycpumap, i, pyused) < 0)
- goto error;
+ PyList_SET_ITEM(pycpumap, i, pyused);
}
if (PyTuple_SetItem(ret, 1, pycpumap) < 0)
--
1.8.3.1
7:35 a.m.
New subject: [libvirt] [PATCH 1/2] python: prefer PyList_SET_ITEM to PyList_SetItem
On Thu, Nov 07, 2013 at 03:44:22PM +0100, Giuseppe Scrivano wrote:
The PyList_SET_ITEM macro, differently from PyList_SetItem,
doesn't do
any error checking and overwrites anything that was previously stored
in the list at the chosen destination position.
PyList_SET_ITEM is usually faster than PyList_SetItem, and since it is
used only to fill freshly created lists, it is safe to be used here.
Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
---
python/libvirt-override.c | 197 +++++++++++++++++++++-------------------------
1 file changed, 90 insertions(+), 107 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 2e58bf9..83bca94 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
[...]
@@ -2575,13 +2569,12 @@ libvirt_virDomainListAllSnapshots(PyObject
*self ATTRIBUTE_UNUSED,
goto cleanup;
for (i = 0; i < c_retval; i++) {
- if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL ||
- PyList_SetItem(py_retval, i, pyobj_snap) < 0) {
- Py_XDECREF(pyobj_snap);
+ if (!(pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i]))) {
Not that it's a problem, just note that you're changing the '== NULL'
to '!' here, but not elsewhere...
Py_DECREF(py_retval);
py_retval = NULL;
goto cleanup;
}
+ PyList_SET_ITEM(py_retval, i, pyobj_snap);
snaps[i] = NULL;
}
[...]
@@ -3219,7 +3209,7 @@ libvirt_virNodeGetCellsFreeMemory(PyObject
*self ATTRIBUTE_UNUSED, PyObject *arg
}
py_retval = PyList_New(c_retval);
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i,
+ PyList_SET_ITEM(py_retval, i,
libvirt_longlongWrap((long long) freeMems[i]));
Pre-existing bad indentation.
}
VIR_FREE(freeMems);
@@ -3398,7 +3388,7 @@ libvirt_virConnectListStoragePools(PyObject *self
ATTRIBUTE_UNUSED,
if (names) {
for (i = 0; i < c_retval; i++) {
- PyList_SetItem(py_retval, i, libvirt_constcharPtrWrap(names[i]));
+ PyList_SET_ITEM(py_retval, i, libvirt_constcharPtrWrap(names[i]));
VIR_FREE(names[i]);
}
VIR_FREE(names);
[...]
@@ -3654,12 +3642,12 @@ libvirt_virStoragePoolGetInfo(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args) {
if ((py_retval = PyList_New(4)) == NULL)
return VIR_PY_NONE;
- PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.state));
- PyList_SetItem(py_retval, 1,
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.state));
+ PyList_SET_ITEM(py_retval, 1,
libvirt_longlongWrap((unsigned long long) info.capacity));
- PyList_SetItem(py_retval, 2,
+ PyList_SET_ITEM(py_retval, 2,
libvirt_longlongWrap((unsigned long long) info.allocation));
- PyList_SetItem(py_retval, 3,
+ PyList_SET_ITEM(py_retval, 3,
libvirt_longlongWrap((unsigned long long) info.available));
return py_retval;
}
Indentation is off after your patch in this whole hunk.
@@ -3685,10 +3673,10 @@ libvirt_virStorageVolGetInfo(PyObject *self
ATTRIBUTE_UNUSED, PyObject *args) {
if ((py_retval = PyList_New(3)) == NULL)
return VIR_PY_NONE;
- PyList_SetItem(py_retval, 0, libvirt_intWrap((int) info.type));
- PyList_SetItem(py_retval, 1,
+ PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.type));
+ PyList_SET_ITEM(py_retval, 1,
libvirt_longlongWrap((unsigned long long) info.capacity));
- PyList_SetItem(py_retval, 2,
+ PyList_SET_ITEM(py_retval, 2,
libvirt_longlongWrap((unsigned long long) info.allocation));
return py_retval;
}
ditto
[...]
... but ACK as-is, because there are more indentation problems,
anyway. So I've prepared a follow-up patch cleaning few bits in this
file.
Martin
8:44 a.m.
New subject: [libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New
Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
---
python/libvirt-override.c | 88 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 70 insertions(+), 18 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 83bca94..3b902bc 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
@@ -2095,7 +2095,7 @@ static int virConnectCredCallbackWrapper(virConnectCredentialPtr
cred,
PyObject *pyauth = (PyObject *)cbdata;
PyObject *pycbdata;
PyObject *pycb;
- PyObject *pyret;
+ PyObject *pyret = NULL;
int ret = -1;
size_t i;
@@ -2108,7 +2108,8 @@ static int virConnectCredCallbackWrapper(virConnectCredentialPtr
cred,
pycred = PyTuple_New(ncred);
for (i = 0; i < ncred; i++) {
PyObject *pycreditem;
- pycreditem = PyList_New(5);
+ if (!(pycreditem = PyList_New(5)))
+ goto cleanup;
Py_INCREF(Py_None);
PyTuple_SetItem(pycred, i, pycreditem);
PyList_SET_ITEM(pycreditem, 0, PyInt_FromLong((long) cred[i].type));
@@ -2382,7 +2383,10 @@ libvirt_virConnectListDomainsID(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ VIR_FREE(ids);
+ return VIR_PY_NONE;
+ }
if (ids) {
for (i = 0; i < c_retval; i++) {
@@ -2472,7 +2476,12 @@ libvirt_virConnectListDefinedDomains(PyObject *self
ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
+ return VIR_PY_NONE;
+ }
if (names) {
for (i = 0; i < c_retval; i++) {
@@ -2621,7 +2630,8 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval)))
+ goto cleanup;
for (i = 0; i < c_retval; i++) {
if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL) {
@@ -2724,7 +2734,8 @@ libvirt_virDomainGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0)
return VIR_PY_NONE;
- py_retval = PyList_New(5);
+ if (!(py_retval = PyList_New(5)))
+ return VIR_PY_NONE;
PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.state));
PyList_SET_ITEM(py_retval, 1, libvirt_ulongWrap(info.maxMem));
PyList_SET_ITEM(py_retval, 2, libvirt_ulongWrap(info.memory));
@@ -2757,7 +2768,9 @@ libvirt_virDomainGetState(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args)
if (c_retval < 0)
return VIR_PY_NONE;
- py_retval = PyList_New(2);
+ if (!(py_retval = PyList_New(2)))
+ return VIR_PY_NONE;
+
PyList_SET_ITEM(py_retval, 0, libvirt_intWrap(state));
PyList_SET_ITEM(py_retval, 1, libvirt_intWrap(reason));
return py_retval;
@@ -2782,7 +2795,8 @@ libvirt_virDomainGetControlInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args)
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0)
return VIR_PY_NONE;
- py_retval = PyList_New(3);
+ if (!(py_retval = PyList_New(3)))
+ return VIR_PY_NONE;
PyList_SET_ITEM(py_retval, 0, libvirt_intWrap(info.state));
PyList_SET_ITEM(py_retval, 1, libvirt_intWrap(info.details));
PyList_SET_ITEM(py_retval, 2, libvirt_longlongWrap(info.stateTime));
@@ -2808,7 +2822,9 @@ libvirt_virDomainGetBlockInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0)
return VIR_PY_NONE;
- py_retval = PyList_New(3);
+ if (!(py_retval = PyList_New(3)))
+ return VIR_PY_NONE;
+
PyList_SET_ITEM(py_retval, 0, libvirt_ulonglongWrap(info.capacity));
PyList_SET_ITEM(py_retval, 1, libvirt_ulonglongWrap(info.allocation));
PyList_SET_ITEM(py_retval, 2, libvirt_ulonglongWrap(info.physical));
@@ -2832,7 +2848,9 @@ libvirt_virNodeGetInfo(PyObject *self ATTRIBUTE_UNUSED, PyObject
*args) {
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0)
return VIR_PY_NONE;
- py_retval = PyList_New(8);
+ if (!(py_retval = PyList_New(8)))
+ return VIR_PY_NONE;
+
PyList_SET_ITEM(py_retval, 0, libvirt_constcharPtrWrap(&info.model[0]));
PyList_SET_ITEM(py_retval, 1, libvirt_longWrap((long) info.memory >> 10));
PyList_SET_ITEM(py_retval, 2, libvirt_intWrap((int) info.cpus));
@@ -2952,7 +2970,12 @@ libvirt_virConnectListNetworks(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
+ return VIR_PY_NONE;
+ }
if (names) {
for (i = 0; i < c_retval; i++) {
@@ -2998,7 +3021,12 @@ libvirt_virConnectListDefinedNetworks(PyObject *self
ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
+ return VIR_PY_NONE;
+ }
if (names) {
for (i = 0; i < c_retval; i++) {
@@ -3207,11 +3235,14 @@ libvirt_virNodeGetCellsFreeMemory(PyObject *self ATTRIBUTE_UNUSED,
PyObject *arg
VIR_FREE(freeMems);
return VIR_PY_NONE;
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval)))
+ goto cleanup;
+
for (i = 0; i < c_retval; i++) {
PyList_SET_ITEM(py_retval, i,
libvirt_longlongWrap((long long) freeMems[i]));
}
+cleanup:
VIR_FREE(freeMems);
return py_retval;
}
@@ -3790,7 +3821,12 @@ libvirt_virNodeListDevices(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
+ return VIR_PY_NONE;
+ }
if (names) {
for (i = 0; i < c_retval; i++) {
@@ -3880,7 +3916,12 @@ libvirt_virNodeDeviceListCaps(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(names[i]);
+ VIR_FREE(names);
+ return VIR_PY_NONE;
+ }
if (names) {
for (i = 0; i < c_retval; i++) {
@@ -4000,7 +4041,12 @@ libvirt_virConnectListSecrets(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(uuids[i]);
+ VIR_FREE(uuids);
+ return VIR_PY_NONE;
+ }
if (uuids) {
for (i = 0; i < c_retval; i++) {
@@ -4219,7 +4265,12 @@ libvirt_virConnectListNWFilters(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval))) {
+ for (i = 0; i < c_retval; i++)
+ VIR_FREE(uuids[i]);
+ VIR_FREE(uuids);
+ return VIR_PY_NONE;
+ }
if (uuids) {
for (i = 0; i < c_retval; i++) {
@@ -4502,7 +4553,8 @@ libvirt_virDomainGetJobInfo(PyObject *self ATTRIBUTE_UNUSED,
PyObject *args) {
LIBVIRT_END_ALLOW_THREADS;
if (c_retval < 0)
return VIR_PY_NONE;
- py_retval = PyList_New(12);
+ if (!(py_retval = PyList_New(12)))
+ return VIR_PY_NONE;
PyList_SET_ITEM(py_retval, 0, libvirt_intWrap((int) info.type));
PyList_SET_ITEM(py_retval, 1, libvirt_ulonglongWrap(info.timeElapsed));
PyList_SET_ITEM(py_retval, 2, libvirt_ulonglongWrap(info.timeRemaining));
--
1.8.3.1
7:53 a.m.
New subject: [libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New
On Thu, Nov 07, 2013 at 03:44:23PM +0100, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan(a)redhat.com>
---
python/libvirt-override.c | 88 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 70 insertions(+), 18 deletions(-)
diff --git a/python/libvirt-override.c b/python/libvirt-override.c
index 83bca94..3b902bc 100644
--- a/python/libvirt-override.c
+++ b/python/libvirt-override.c
[...]
@@ -2621,7 +2630,8 @@
libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED,
return VIR_PY_NONE;
}
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval)))
+ goto cleanup;
This function should follow others and return VIR_PY_NONE, but cleanup
path returns py_retval.
for (i = 0; i < c_retval; i++) {
if ((pyobj_snap = libvirt_constcharPtrWrap(names[i])) == NULL) {
[...]
@@ -3207,11 +3235,14 @@ libvirt_virNodeGetCellsFreeMemory(PyObject
*self ATTRIBUTE_UNUSED, PyObject *arg
VIR_FREE(freeMems);
return VIR_PY_NONE;
}
- py_retval = PyList_New(c_retval);
+ if (!(py_retval = PyList_New(c_retval)))
+ goto cleanup;
+
ditto
for (i = 0; i < c_retval; i++) {
PyList_SET_ITEM(py_retval, i,
libvirt_longlongWrap((long long) freeMems[i]));
}
+cleanup:
VIR_FREE(freeMems);
return py_retval;
}
ACK with those two places fixed (don't forget to free the names in a
loop in the first hunk).
Martin
8 a.m.
New subject: [libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New
On 11/12/2013 06:53 AM, Martin Kletzander wrote:
> @@ -2621,7 +2630,8 @@
libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED,
> return VIR_PY_NONE;
> }
> }
> - py_retval = PyList_New(c_retval);
> + if (!(py_retval = PyList_New(c_retval)))
> + goto cleanup;
>
This function should follow others and return VIR_PY_NONE, but cleanup
path returns py_retval.
Actually, you WANT to return NULL, not VIR_PY_NONE, when PyList_New()
fails. Returning NULL is the hint to python to report the OOM
exception; returning VIR_PY_NONE is not NULL and therefore silently
papers over the exception. Worse, the rest of the libvirt python code
treats a return of the python object 'None' as meaning 'the API call
failed, dig out the last virError and turn it into a python exception';
but in this case, there is no virError (our failure was not related to a
failed C API call). This code rewrite is correct as-is.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:50 a.m.
New subject: [libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New
On Tue, Nov 12, 2013 at 07:00:30AM -0700, Eric Blake wrote:
On 11/12/2013 06:53 AM, Martin Kletzander wrote:
>> @@ -2621,7 +2630,8 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
>> return VIR_PY_NONE;
>> }
>> }
>> - py_retval = PyList_New(c_retval);
>> + if (!(py_retval = PyList_New(c_retval)))
>> + goto cleanup;
>>
>
> This function should follow others and return VIR_PY_NONE, but cleanup
> path returns py_retval.
Actually, you WANT to return NULL, not VIR_PY_NONE, when PyList_New()
fails. Returning NULL is the hint to python to report the OOM
exception; returning VIR_PY_NONE is not NULL and therefore silently
papers over the exception. Worse, the rest of the libvirt python code
treats a return of the python object 'None' as meaning 'the API call
failed, dig out the last virError and turn it into a python exception';
but in this case, there is no virError (our failure was not related to a
failed C API call). This code rewrite is correct as-is.
Oh, I learned something new again. Thanks for the explanation. In
that case, I'm afraid there are more places where VIR_PY_NONE is returned
instead of NULL when allocation failed. I'll check the code and add
it to my whitespace cleanup.
Martin
10:37 a.m.
New subject: [libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New
On 11/12/2013 07:00 AM, Eric Blake wrote:
On 11/12/2013 06:53 AM, Martin Kletzander wrote:
>> @@ -2621,7 +2630,8 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self
ATTRIBUTE_UNUSED,
>> return VIR_PY_NONE;
>> }
>> }
>> - py_retval = PyList_New(c_retval);
>> + if (!(py_retval = PyList_New(c_retval)))
>> + goto cleanup;
>>
>
> This function should follow others and return VIR_PY_NONE, but cleanup
> path returns py_retval.
Actually, you WANT to return NULL, not VIR_PY_NONE, when PyList_New()
fails. Returning NULL is the hint to python to report the OOM
exception;
Re-reading what I wrote, I kind of implied that OOM error will always be
reported for a NULL return; but that's not true - OOM error is the most
likely case of failure for PyList_New(), but other calls definitely can
have non-OOM reasons for returning NULL. The more general rule is that
returning NULL says that python should raise the current python
thread-local error information as an exception. Most Py* API are
documented as setting thread-local error information (whether OOM or
otherwise) before returning NULL. If you have a situation where you are
returning NULL to raise a python exception, but did not obtain that NULL
from a Py* call, then you have to explicitly set the thread-local error
(see for example getPyVirTypedParameter, which gets to reuse PyInt_*
conversions in most cases, but has to explicitly call PyErr_Format() for
a non-OOM error in the default case; see also the use of
PyErr_NoMemory() in setPyVirTypedParameter as a way to explicitly
request an OOM error when the allocation failure was not due to any
other Py* call).
But the general observation remains - we have a lot of crufty code that
does error handling incorrectly in our python bindings :) And there's
still the question of whether we are going to split python bindings into
their own repository; it might be nice to have the code cleaned up
before that point.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:18 a.m.
New subject: [libvirt] [PATCH 2/2] python: add error checking for calls to PyList_New
Eric Blake <eblake(a)redhat.com> writes:
But the general observation remains - we have a lot of crufty code
that
does error handling incorrectly in our python bindings :) And there's
still the question of whether we are going to split python bindings into
their own repository; it might be nice to have the code cleaned up
before that point.
I forgot to add that I have deliberately left the function
"libvirt_virConnectDomainEventGraphicsCallback" unchanged in my patch
(probably this should go in the commit message), that I prefer to
address later. It requires a bigger refactoring; from what I can see,
it expects things just to work.
Giuseppe
4029
days inactive
4034
days old
8 comments
3 participants
participants (3)
-
Eric Blake -
Giuseppe Scrivano -
Martin Kletzander