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 <[email protected]> > --- > 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
signature.asc
Description: Digital signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
