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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list