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

2015-03-25 Thread Huanle Han 
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='0x' 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 jfer...@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 hanxue...@gmail.com mailto:
 hanxue...@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 ||

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

2015-03-24 Thread John Ferlan 


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 hanxue...@gmail.com mailto:hanxue...@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

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

2015-03-23 Thread Huanle Han 
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 hanxue...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list