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

[John Ferlan]

On 03/10/2016 02:25 PM, John Ferlan wrote:
> 
> 
> On 03/10/2016 01:54 PM, John Ferlan wrote:
>>
>>
>> On 03/07/2016 12:24 PM, Andrea Bolognani wrote:
>>> virHostdevGetPCIHostDeviceList() is similar but does not filter out
>>> devices that are not in the active list; that said, we are looking
>>> up the device in the active list just a few lines after anyway, so
>>> we might as well just keep a single function around.
>>>
>>> This also helps stress the fact the objects contained in pcidevs are
>>> only for looking up the actual devices, which is something later
>>> commits will make even more explicit.
>>> ---
>>>  src/util/virhostdev.c | 50 ++++----------------------------------------------
>>>  1 file changed, 4 insertions(+), 46 deletions(-)
>>>

Please read my logic in patch 20 before doing anything - I'm in the
middle of it... scroll down for my short thoughts [1]...

John
> 
> <SIGH>  Should have read patch 18 & 19 first... Looks like I'm getting
> confuzzled by all these lists and multitude of ways names were
> generated. The comparison being done is against the copy that came from
> the activePCIHostdev list which will have the fields I was concerned
> about. So in retrospect...
> 
> ACK
> 
> John
>>
>>
>> Existing code uses virPCIDeviceListAddCopy (as does code that populates
>> inactiveDevs list).  The AddCopy code will
>>
>> 1. virPCIDeviceCopy(virPCIDevicePtr dev)
>>    VIR_ALLOC(copy);
>>    *copy = *dev;
>>    Update copy->path, copy->used_by_drvname, & copy->used_by_domname
>>
>> 2. Add to a virPCIDeviceListPtr
>>
>> The new code.
>>
>> 1. virPCIDeviceNew for a virPCIDevicePtr pci;
>>    VIR_ALLOC(dev);
>>    copy in dev->address
>>    generate dev->name, dev->id (similar to copy)
>>    generate dev->path from dev->name
>>
>> 2. Add to a virPCIDeviceListPtr
>>
>> 3. Caller sets the managed and stubDriver backend.
>>
>> NOTE: there's no copy of 'used_by_drvname' and 'used_by_domname', like
>> the copy function nor are a few other fields set.  This seems to be
>> important later [1].
>>
>>> diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
>>> index fef7898..67e6e7b 100644
>>> --- a/src/util/virhostdev.c
>>> +++ b/src/util/virhostdev.c
>>> @@ -245,49 +245,6 @@ virHostdevGetPCIHostDeviceList(virDomainHostdevDefPtr *hostdevs, int nhostdevs)
>>>  }
>>>  
>>>  
>>> -/*
>>> - * virHostdevGetActivePCIHostDeviceList - make a new list with a *copy* of
>>> - *   every virPCIDevice object that is found on the activePCIHostdevs
>>> - *   list *and* is in the hostdev list for this domain.
>>> - *
>>> - * Return the new list, or NULL if there was a failure.
>>> - *
>>> - * Pre-condition: activePCIHostdevs is locked
>>> - */
>>> -static virPCIDeviceListPtr
>>> -virHostdevGetActivePCIHostDeviceList(virHostdevManagerPtr mgr,
>>> -                                     virDomainHostdevDefPtr *hostdevs,
>>> -                                     int nhostdevs)
>>> -{
>>> -    virPCIDeviceListPtr list;
>>> -    size_t i;
>>> -
>>> -    if (!(list = virPCIDeviceListNew()))
>>> -        return NULL;
>>> -
>>> -    for (i = 0; i < nhostdevs; i++) {
>>> -        virDomainHostdevDefPtr hostdev = hostdevs[i];
>>> -        virDevicePCIAddressPtr addr;
>>> -        virPCIDevicePtr activeDev;
>>> -
>>> -        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
>>> -            continue;
>>> -        if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI)
>>> -            continue;
>>> -
>>> -        addr = &hostdev->source.subsys.u.pci.addr;
>>> -        activeDev = virPCIDeviceListFindByIDs(mgr->activePCIHostdevs,
>>> -                                              addr->domain, addr->bus,
>>> -                                              addr->slot, addr->function);
>>> -        if (activeDev && virPCIDeviceListAddCopy(list, activeDev) < 0) {
>>> -            virObjectUnref(list);
>>> -            return NULL;
>>> -        }
>>> -    }
>>> -
>>> -    return list;
>>> -}
>>> -
>>>  static int
>>>  virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev,
>>>                         char **sysfs_path)
>>> @@ -800,9 +757,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
>>>      virObjectLock(hostdev_mgr->activePCIHostdevs);
>>>      virObjectLock(hostdev_mgr->inactivePCIHostdevs);
>>>  
>>> -    if (!(pcidevs = virHostdevGetActivePCIHostDeviceList(hostdev_mgr,
>>> -                                                         hostdevs,
>>> -                                                         nhostdevs))) {
>>> +    if (!(pcidevs = virHostdevGetPCIHostDeviceList(hostdevs, nhostdevs))) {
>>>          virErrorPtr err = virGetLastError();
>>>          VIR_ERROR(_("Failed to allocate PCI device list: %s"),
>>>                    err ? err->message : _("unknown error"));
>>> @@ -832,6 +787,9 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr hostdev_mgr,
>>
>> [1] This code checks for usedby_drvname and usedby_domname
>>
>> These are set by virHostdevPreparePCIDevices and
>> virHostdevUpdateActivePCIDevices.  The virHostdevPreparePCIDevices is
>> the only current caller of virHostdevGetPCIHostDeviceList.  The latter
>> will call the virPCIDeviceNew and then set the Managed, UsedBy, and
>> stubDriver fields.
>>
>> The PreparePCIDevices code seems to do many of the same functions for
>> the activePCIHostdevs.
>>
>> I think this one needs to be rethought..
>>
>>
>> John
>>>                  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!
>>>
>>