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.