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

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



On 2012年12月05日 01:48, Jiri Denemark wrote:
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

Hum, I ignored this pciDeviceListStealIndex changing when reviewing. ACK
then.

Osier


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