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