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

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