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

[Osier Yang]

于 2011年10月13日 09:05, 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).

Ok, I made two different versions, one keeps using 2 list, while another
uses just one. Let's choose. :-)

Osier