[libvirt] [PATCH] 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@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..1e1a2ee 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 Tue, Nov 05, 2013 at 12:49:19PM +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..1e1a2ee 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]))) {
You missed ' == NULL' in here!
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);
Not that it has any connection to your patch, but I noticed that, somewhere, we check the return value of PyList_New(), but somewhere we don't... I guess we should do it everywhere, shouldn't we? ... thinking about it, I searched through the code and PyList_SetItem() properly errors out when 'op' (its first param) is NULL, but PyList_SET_ITEM() will just segfault. I think we should properly handle allocation errors before optimizing it this way. Martin
Martin Kletzander <mkletzan@redhat.com> writes:
Not that it has any connection to your patch, but I noticed that, somewhere, we check the return value of PyList_New(), but somewhere we don't... I guess we should do it everywhere, shouldn't we?
... thinking about it, I searched through the code and PyList_SetItem() properly errors out when 'op' (its first param) is NULL, but PyList_SET_ITEM() will just segfault. I think we should properly handle allocation errors before optimizing it this way.
Great you have noticed it. I expected PyList_SetItem to crash too when the list == NULL. Thanks for the review. Giuseppe
On Tue, Nov 05, 2013 at 05:00:53PM +0100, Giuseppe Scrivano wrote:
Martin Kletzander <mkletzan@redhat.com> writes:
Not that it has any connection to your patch, but I noticed that, somewhere, we check the return value of PyList_New(), but somewhere we don't... I guess we should do it everywhere, shouldn't we?
... thinking about it, I searched through the code and PyList_SetItem() properly errors out when 'op' (its first param) is NULL, but PyList_SET_ITEM() will just segfault. I think we should properly handle allocation errors before optimizing it this way.
Great you have noticed it. I expected PyList_SetItem to crash too when the list == NULL.
Oh, you're right. I was blinded by the fact, that PyList_SetItem() checks whether the 'op' is a list and I was under the impression that PyList_Check(op) will catch that. This has not yet changed (I checked 3.3.2), so you're right, the fact that we'll use PyList_SET_ITEM won't change a thing. I'll continue with the review in a minute. Thanks for the idea. Martin
Martin Kletzander <mkletzan@redhat.com> writes:
Oh, you're right. I was blinded by the fact, that PyList_SetItem() checks whether the 'op' is a list and I was under the impression that PyList_Check(op) will catch that. This has not yet changed (I checked 3.3.2), so you're right, the fact that we'll use PyList_SET_ITEM won't change a thing. I'll continue with the review in a minute. Thanks for the idea.
If you haven't done the review yet, I can send you a new version in a while that adds error checking for PyList_New. If you already worked on the review, then I can split these additional changes and rebase them. Thanks, Giuseppe
Giuseppe Scrivano <gscrivan@redhat.com> writes:
Martin Kletzander <mkletzan@redhat.com> writes:
Oh, you're right. I was blinded by the fact, that PyList_SetItem() checks whether the 'op' is a list and I was under the impression that PyList_Check(op) will catch that. This has not yet changed (I checked 3.3.2), so you're right, the fact that we'll use PyList_SET_ITEM won't change a thing. I'll continue with the review in a minute. Thanks for the idea.
If you already worked on the review, then I can split these additional changes and rebase them.
I'll send these changes as a separate series. Giuseppe
participants (2)
-
Giuseppe Scrivano -
Martin Kletzander