[libvirt] [PATCH] python: prefer PyList_SET_ITEM to PyList_SetItem

[Martin Kletzander]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20131105/939b4ca1/attachment-0001.sig>