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