Re: [libvirt] [PATCH v3 5/5] python: Add binding for virDomainGetDiskErrors
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)