Re: [libvirt] [PATCH 15/24] hostdev: Remove virHostdevGetActivePCIHostDeviceList()

On Fri, 2016-03-11 at 14:01 -0500, John Ferlan wrote: > Please read my logic in patch 20 before doing anything - I'm in the > middle of it... scroll down for my short thoughts [1]... Your comments to patch 20 are extremely detailed, so it will take me a while to go through them and reply properly. In the meantime, I'll give a short reply to your short thoughts :) >>>> virPCIDeviceListDel(pcidevs, dev); >>>> continue; > [1] this... > >>>> } >>>> + } else { >>>> + virPCIDeviceListDel(pcidevs, dev); >>>> + continue; > [1] ... and this mean... > >>>> } >>>> >>>> VIR_DEBUG("Removing PCI device %s from active list", > [1] ... this doesn't happen! The code goes like this: activeDev = virPCIDeviceListFind(mgr->activePCIHostdevs, dev); if (activeDev) { const char *usedby_drvname; const char *usedby_domname; virPCIDeviceGetUsedBy(activeDev, &usedby_drvname, &usedby_domname); if (STRNEQ_NULLABLE(drv_name, usedby_drvname) || STRNEQ_NULLABLE(dom_name, usedby_domname)) { virPCIDeviceListDel(pcidevs, dev); continue; } } else { virPCIDeviceListDel(pcidevs, dev); continue; } VIR_DEBUG("Removing PCI device %s from active list", virPCIDeviceGetName(dev)); virPCIDeviceListDel(mgr->activePCIHostdevs, dev); i++; So 'dev' is used too look up 'activeDev' (should be 'actual', but the variable renaming patch has not been applyed at this point in time) in the active list. If no match is found, it means that the PCI device 'dev' is referring to is not actually active, and we should remove it from 'pcidevs' (so that it doesn't get processed further) and move on to the next one. If a match is found, but the actual device is used either by a different driver or by a different domain, we do the same thing: remove 'dev' from 'pcidevs' and move on. If neither of the above is true, namely if the device *is* active and *is* used by the driver and domain we're processing, *then* we proceed to remove it from the active list and, later, reattach it to the host. It should be noted that "making sure the devices we're about to reattach to the host are the ones this domain is using" and "remove devices from the active list" are split in patch 22, with the latter being joined with "add devices to the inactive list". All in all, I believe this is still correct and can be pushed along with patches 18 and 19 (after taking care of your comments there) before moving on with the more troublesome patch 20. Let me know whether you agree :)