Re: [libvirt] [PATCH] hostdev: fix two bugs in virHostdevReAttachPCIDevices

On 03/22/2015 09:59 AM, Huanle Han wrote: > > Bug 1: The the next element in the pcidevs is skipped after we > virPCIDeviceListDel the previous element. > > Bug 2: virHostdevNetConfigRestore is called for store the hostdevs > which may be used by other domain. > > Signed-off-by: Huanle Han <hanxueluo(a)gmail.com <mailto: hanxueluo(a)gmail.com&gt;&gt; > --- > src/util/virhostdev.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > Should this be two separate patches? IOW: Are you fixing two separate issues or essentially the same issue? Makes it easier to cherry-pick and choose what may need to be backported elsewhere... If there's a bug associated that would have been "good" to list as well as perhaps an example or two... That is - what XML and command(s) sequence(s) prompted you to write the patch(es)... Also, my attempt to git am -3 your patch fails - perhaps it's your mailer configuration. Did you use git send-email? or something else... I'm seeing html format... So I'm only reviewing based on what I read. > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c > index 9678e2b..ecbe5d8 100644 > --- a/src/util/virhostdev.c > +++ b/src/util/virhostdev.c > @@ -785,7 +785,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr > hostdev_mgr, > * them and reset all the devices before re-attach. > * Attach mac and port profile parameters to devices > */ ^^^ I think the comment here could be updated to make it clear what's being done. To just say "Again 4 loops;..." is a bit misleading since the only loops I found in the code were in virHostdevPreparePCIDevices where there are (currently) 9 such loops! > - for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { > + for (i = 0; i < virPCIDeviceListCount(pcidevs);) { > virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); > virPCIDevicePtr activeDev = NULL; > > @@ -806,6 +806,7 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr > hostdev_mgr, > } > > virPCIDeviceListDel(hostdev_mgr->activePCIHostdevs, dev); > + i++; > } Not sure I'm seeing why your fix works... Are you claiming that the remove from other domain check (e.g. where the code "continue;"'s) shouldn't be updating the 'i' value? All it seems you did was move the i++ from the for line to after the second virPCIDeviceListDel in the block... Which yes, does reduce the count of devices and I would think should be the case that doesn't increment i... If we have 4 devices [0, 1, 2, 3] and we remove [1], then there are 3 devices left and i would be 2, but the new array would be [0, 2, 3] and thus we don't check [2]. Perhaps this would work/read better as a while loop. > > /* At this point, any device that had been used by the guest is in > @@ -816,9 +817,25 @@ virHostdevReAttachPCIDevices(virHostdevManagerPtr > hostdev_mgr, > * For SRIOV net host devices, unset mac and port profile before > * reset and reattach device > */ > - for (i = 0; i < nhostdevs; i++) > - virHostdevNetConfigRestore(hostdevs[i], hostdev_mgr->stateDir, > - oldStateDir); > + for (i = 0; i < nhostdevs; i++) { > + virPCIDevicePtr dev; > + virDomainHostdevDefPtr hostdev = hostdevs[i]; > + virDomainHostdevSubsysPCIPtr pcisrc = > &hostdev->source.subsys.u.pci; > + > + if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS || > + hostdev->source.subsys.type != > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI || > + hostdev->parent.type != VIR_DOMAIN_DEVICE_NET || > + !hostdev->parent.data.net <http://parent.data.net>) ^^^^^^^^^^^^^^^^^^^^^^^^ This is what makes me believe you sent in html format It's also directly taken from virHostdevNetConfigRestore... and could be considered partially from virHostdevPreparePCIDevices Suggestion - create a patch that has a new static bool function ("isHostdevPCINetDevice") that does the same check passing "hostdev"... Then adjust the (currently) two places that make that check in order to use the bool function to decide to continue or not. Then this patch would use that function here instead. > + continue; > + > + dev = virPCIDeviceNew(pcisrc->addr.domain, pcisrc->addr.bus, > + pcisrc->addr.slot, pcisrc->addr.function); > + if (virPCIDeviceListFind(pcidevs, dev)) { > + virHostdevNetConfigRestore(hostdev, hostdev_mgr->stateDir, > + oldStateDir); > + } > + virPCIDeviceFree(dev); > + } It's still not quite clear to me from your description why we have to search the pcidevs for our device before calling the Restore function. Does it matter if it's in or not in the activePCIHostdevs list (the comment just before this section). I think this is one of those cases where the code perhaps isn't self documenting and that by adding a few more comments along the way we can better describe what we're trying to do and why it has to be done. John > > for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { > virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); > -- > 2.1.0 > > > > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >