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

[John Ferlan]

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