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

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(-) >>

<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;

>> } >> + } else { >> + virPCIDeviceListDel(pcidevs, dev); >> + continue;

>> } >> >> VIR_DEBUG("Removing PCI device %s from active list",

>> >