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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]