Re: [PATCH 3/4] qemu: Fix memory (and FD) leak on PCI device detach

[Jiri Denemark <jdenemar@xxxxxxxxxx>]

On Tue, Dec 04, 2012 at 22:29:23 +0800, Osier Yang wrote:
> On 2012年12月04日 18:38, Jiri Denemark wrote:
> > Unmanaged PCI devices were only leaked if pciDeviceListAdd failed but
> > managed devices were always leaked. And leaking PCI device is likely to
> > leave PCI config file descriptor open. This patch fixes
> > qemuReattachPciDevice to either free the PCI device or add it to the
> > inactivePciHostdevs list.
> > ---
> >   src/qemu/qemu_hostdev.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> > index b79319e..a748b8b 100644
> > --- a/src/qemu/qemu_hostdev.c
> > +++ b/src/qemu/qemu_hostdev.c
> > @@ -546,10 +546,8 @@ int qemuPrepareHostdevPCIDevices(virQEMUDriverPtr driver,
> >       }
> >
> >       /* Loop 9: Now steal all the devices from pcidevs */
> > -    while (pciDeviceListCount(pcidevs)>  0) {
> > -        pciDevice *dev = pciDeviceListGet(pcidevs, 0);
> > -        pciDeviceListSteal(pcidevs, dev);
> > -    }
> > +    while (pciDeviceListCount(pcidevs)>  0)
> > +        pciDeviceListStealIndex(pcidevs, 0);
> >
> >       ret = 0;
> >       goto cleanup;
> > @@ -818,7 +816,8 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr driver)
> >        * successfully, it must have been inactive.
> >        */
> >       if (!pciDeviceGetManaged(dev)) {
> > -        pciDeviceListAdd(driver->inactivePciHostdevs, dev);
> > +        if (pciDeviceListAdd(driver->inactivePciHostdevs, dev)<  0)
> > +            pciFreeDevice(dev);
> >           return;
> >       }
> >
> > @@ -835,6 +834,7 @@ void qemuReattachPciDevice(pciDevice *dev, virQEMUDriverPtr driver)
> >                     err ? err->message : _("unknown error"));
> >           virResetError(err);
> >       }
> > +    pciFreeDevice(dev);
> >   }
> 
> This produces the similar problem 1/4 tries to fix. pciDeviceListFree
> right after will free the whole list. Except this, the using of
> pciDeviceListStealIndex is nice.

I don't think it does. There are only two places where qemuReattachPciDevice
is called. The first one is the hunk you removed from this patch:

    @@ -905,8 +905,8 @@ void qemuDomainReAttachHostdevDevices(virQEMUDriverPtr driver,
             }
         }
     
    -    for (i = 0; i < pciDeviceListCount(pcidevs); i++) {
    -        pciDevice *dev = pciDeviceListGet(pcidevs, i);
    +    while (pciDeviceListCount(pcidevs) > 0) {
    +        pciDevice *dev = pciDeviceListStealIndex(pcidevs, 0);
             qemuReattachPciDevice(dev, driver);
         }
     

and its purpose is to fix the caller to call qemuReattachPciDevice with a
device that is not referenced from pcidevs list (hence, it changes
pciDeviceListGet into pciDeviceListStealIndex). And the second one is in
qemu_hotplug.c:

        activePci = pciDeviceListSteal(driver->activePciHostdevs, pci);
        if (activePci &&
            pciResetDevice(activePci, driver->activePciHostdevs,
                           driver->inactivePciHostdevs) == 0) {
            qemuReattachPciDevice(activePci, driver);
        } else {
            /* reset of the device failed, treat it as if it was returned */
            pciFreeDevice(activePci);
            ret = -1;
        }

That is, it has already been relying on qemuReattachPciDevice to properly free
activePci if needed.

Jirka

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list