[libvirt] [PATCH 0/2] libvirt-override.c: Use PyList_SET_ITEM + error checking for PyList_New
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
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@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
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@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
Signed-off-by: Giuseppe Scrivano <gscrivan@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
On Thu, Nov 07, 2013 at 03:44:23PM +0100, Giuseppe Scrivano wrote:
Signed-off-by: Giuseppe Scrivano <gscrivan@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
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
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
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
Eric Blake <eblake@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
participants (3)
-
Eric Blake -
Giuseppe Scrivano -
Martin Kletzander