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!
>>>
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list