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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list