[libvirt] [PATCH] qemu: Get the dev from activePciHostdevs list before reattachment

[Osier Yang]

于 2011年10月13日 01:31, Eric Blake 写道:
> On 10/11/2011 01:59 AM, Osier Yang wrote:
>> BZ# https://bugzilla.redhat.com/show_bug.cgi?id=736214
>>
>> The problem is caused by the original info of domain's PCI dev is
>> maintained by qemu_driver->activePciHostdevs list, (E.g. dev->reprobe,
>> which stands for whether need to reprobe driver for the dev when do
>> reattachment). The fields (dev->reprobe, dev->unbind_from_stub, and
>> dev->remove_slot) are initialized properly when preparing the PCI
>> device for managed attachment. However, when do reattachment, it
>> construct a complete new "pciDevice" without honoring the original
>> dev info, and thus the dev won't get the original driver or can get
>> other problem.
>>
>> This patch is to fix the problem by get the dev from list
>> driver->activePciHostdevs if it's in the list, though it's unlikely
>> the dev to be reattached is not in the list, as any PCI dev is added
>> to the list when do preparation (qemuPrepareHostdevPCIDevices),
>> the patch doesn't completely desert the old design so the device
>> can be reattached even if it's not in list driver->activePciHostdevs.
>>
>> @@ -272,12 +273,23 @@ void qemuDomainReAttachHostdevDevices(struct 
>> qemud_driver *driver,
>> return;
>> }
>>
>> + if (!(pci_list = pciDeviceListNew()))
>> + return;
>
> Memory leak. You must goto cleanup, so that pcidevs is not lost.
>
>> +
>> /* Again 3 loops; mark all devices as inactive before reset
>> * them and reset all the devices before re-attach */
>
> Comment says 3 loops, but you added a fourth one (although 
> conceptually, the last two loops are really a combination of one visit 
> to the complete set of devices, divided into two subsets according to 
> whether we are reusing active information).
>
>> -
>> for (i = 0; i< pciDeviceListCount(pcidevs); i++) {
>> pciDevice *dev = pciDeviceListGet(pcidevs, i);
>> - pciDeviceListDel(driver->activePciHostdevs, dev);
>> + pciDevice *pci = NULL;
>> + pci = pciDeviceListSteal(driver->activePciHostdevs, dev);
>> +
>> + if (pci) {
>> + pciDeviceListDel(pcidevs, dev);
>> + if (pciDeviceListAdd(pci_list, pci)< 0) {
>> + pciFreeDevice(pci);
>> + goto cleanup;
>> + }
>> + }
>> }
>
> I'm wondering if all this transfer work between two lists is really 
> necessary, or if there is a more efficient way to just copy the 
> information from driver->activePciHostdevs back into pcidevs, without 
> having to create a second pci_list.
>

I was wondering this too, definitely it will be more efficient to just 
use one
list, but you known, I'm afraid it could break something, though it's 
unlikely
currently, as qemuDomainReAttachHostdevDevices is only used by
qemuDomainAttachHostPciDevice (when attaching a PCI), and qemuProcessStop,
(when destroying a domain, or fails on starting the domain). This means the
all the devices which will be reattached by qemuDomainReAttachHostdevDevices
are already initialized as an element of driver->activePciHostdevs during
preparation (qemuDomainReAttachHostdevDevices).

Osier