[libvirt] [PATCH v3 5/5] python: Add binding for virDomainGetDiskErrors

Jiri Denemark jdenemar at redhat.com
Wed Feb 1 09:16:46 UTC 2012


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




More information about the libvir-list mailing list