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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list