[libvirt] [PATCH] hostdev: fix two bugs in virHostdevReAttachPCIDevices
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@gmail.com> --- src/util/virhostdev.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) 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 */ - 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++; } /* 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) + 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); + } for (i = 0; i < virPCIDeviceListCount(pcidevs); i++) { virPCIDevicePtr dev = virPCIDeviceListGet(pcidevs, i); -- 2.1.0
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@gmail.com <mailto:hanxueluo@gmail.com>> --- 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I'm sorry that I didn't configure the 'git send-email' well. The 2 issues can be consider separate. Issue 1(move the 'i++'): Yes, I do mean that we shouldn't update the 'i' value for the reason you said. I was thinking to minimize the change. But while loop is also a good choose. Issue 2: Steps to reproduce the bug : 1. Domain A and B xml contain the same SRIOV net hostdev(same pci address). ' <interface type='hostdev' managed='yes'> <source> <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> </source> <mac address='52:11:11:22:22:22'/> </interface>' 2. virsh start A (Successfully, and configure the SRIOV net mac witch custom one ) 3. virsh start B (Fail because the hostdev is used by domain A or other reason.) The failure in step 3 makes the mac/vlan of the net in Doamin A change. Because when we call 'virHostdevReAttachPCIDevices' for domain B in step 3, we do 'virHostdevNetConfigRestore' for all net hostdev, even the one is being used by Domain A. The mac/vlan of the SRIOV net back to the value before step 2. In my fix, as the pci used by other domain have been removed from 'pcidevs' in loop 1, we only need do 'virHostdevNetConfigRestore' for the hostdev still in 'pcidevs'(used by current domain). I will resend a patch. Thanks for your suggestion. 2015-03-24 20:47 GMT+08:00 John Ferlan <jferlan@redhat.com>:
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@gmail.com <mailto:
hanxueluo@gmail.com>>
--- 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Huanle Han -
John Ferlan