On 03/14/2016 11:42 AM, Andrea Bolognani wrote:
> 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 :)
>
I probably changed my mind 20 times while reviewing 20-22. I think in
retrospect my secondary comments were incorrect since we could get to
that removal of the device from the active list if the drv_name and
dom_name match.
I think later when we move from activeList to inactiveList things are a
bit clearer, but like we've already agreed upon - this is code that once
you step in it, it's hard to get it off the bottom of your foot.
So, I agree let's ACK this one and move on. It's worth noting that once
this patch is complete 'pcidevs' will be the list of activeDevs that
were removed.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list