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

[Osier Yang]

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