[libvirt] [PATCH v3 5/5] python: Add binding for virDomainGetDiskErrors

[Eric Blake]

On 01/31/2012 12:26 PM, Jiri Denemark wrote:
> ---
>  python/libvirt-override-api.xml |    6 ++++
>  python/libvirt-override.c       |   50 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 0 deletions(-)

> +    if ((count = virDomainGetDiskErrors(domain, NULL, 0, 0)) < 0)
> +        return VIR_PY_NONE;

This one's good.

> +    ndisks = count;
> +
> +    if (ndisks) {
> +        if (!(disks = malloc(sizeof(*disks) * ndisks)))
> +            return VIR_PY_NONE;

You're not the first offender, so I don't care if you check this in
as-is and let a subsequent patch clean this up, but this should be a
place where we return a python OOM exception rather than None.

> +
> +        LIBVIRT_BEGIN_ALLOW_THREADS;
> +        count = virDomainGetDiskErrors(domain, disks, ndisks, 0);
> +        LIBVIRT_END_ALLOW_THREADS;
> +
> +        if (count < 0)
> +            goto cleanup;
> +    }
> +
> +    if (!(py_retval = PyDict_New()))
> +        goto cleanup;

This properly propagates the python exception.

> +
> +    for (i = 0; i < count; i++) {
> +        PyDict_SetItem(py_retval,
> +                       libvirt_charPtrWrap(disks[i].disk),
> +                       libvirt_intWrap(disks[i].error));

Also a case where you're not the first offender (so fixing it can be
saved for a later global cleanup), but we should: 1. check that
libvirt_charPtrWrap() and libvirt_intWrap() aren't returning NULL (since
PyDict_SetItem doesn't handle NULL well), and 2. check for
PyDict_SetItem failures (in which case we free the portion of the
dictionary collected so far and propagate the python exception).

> +    }
> +
> +cleanup:
> +    free(disks);

I think this is a memory leak - if I'm correct, libvirt_charPtrWrap
creates a new object that copies the incoming string to the new object's
content, rather than stealing a reference to your malloc'd string.  That
means you need to loop over count and free each disk name before freeing
disks.

ACK with the memory leak in cleanup: fixed; you can leave the other
issues for a later global cleanup of the python overrides.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120131/303c28d2/attachment-0001.sig>