On Tue, Jan 31, 2012 at 20:57:54 -0700, Eric Blake wrote:
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.
Yeah, I hate creating python binding so I just copied&pasted the boring parts of this function from other places :-)
+ + 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's what libvirt_constcharPtrWrap does. libvirt_charPtrWrap is nice that it also calls free() on the original string at the end. But we can get to cleanup without going through the for loop so I need to change it to call libvirt_constcharPtrWrap and free the disks here. Thanks for pointing that out. Jirka