Change in vdsm[master]: Minor optimization for delNetwork

[Igor Lvovsky]

Igor Lvovsky has uploaded a new change for review.

Change subject: Minor optimization for delNetwork
......................................................................

Minor optimization for delNetwork

Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Signed-off-by: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
---
M vdsm/configNetwork.py
1 file changed, 19 insertions(+), 10 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/07/5207/1
--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;

[Igor Lvovsky]

Igor Lvovsky has posted comments on this change.

Change subject: Minor optimization for delNetwork
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File vdsm/configNetwork.py
Line 794:             ifdown(bonding)
It is exactly same condition, isn't it?
So, why check it twice?

Line 798:         nicUsers = nicOtherUsers(network, vlan, bonding, nic)
which changes?

Line 801:         if bridged and nicUsers:
First, I just keep exactly previous behaviour (except of calling nicOtherUsers once and
not twice ).
But, let see....     
do you mean that at this point we shouldn't be care about bridge because we removed it
several lines above?  
Hmmm, if so, maybe you are right we can avoid questions about bridge.
you are also right in second part, indeed we can put the action under condition

--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Minor optimization for delNetwork
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(4 inline comments)

....................................................
File vdsm/configNetwork.py
Line 774:     # We need to gather NetInfo again to refresh networks info from the
libvirt.
s/the libvirt/libvirt/

Line 794:             ifdown(bonding)
was this removal intentional?

Line 798:         nicUsers = nicOtherUsers(network, vlan, bonding, nic)
I see here an optimization, but there are several other confusing changes...

Line 801:         if bridged and nicUsers:
Why removing the nic depends on whether the net is bridged? (maybe you fix it in a
following patch)

and why do we 'continue' instead of placing the action under condition?

--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Minor optimization for delNetwork
......................................................................


Minor optimization for delNetwork

Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Signed-off-by: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
---
M vdsm/configNetwork.py
1 file changed, 19 insertions(+), 12 deletions(-)

Approvals:
  Lei Li: Verified; Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved


--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Minor optimization for delNetwork
......................................................................


Patch Set 2: Looks good to me, approved

--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;

Lei Li has posted comments on this change.

Change subject: Minor optimization for delNetwork
......................................................................


Patch Set 2: Verified; Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Lei Li <lilei(a)linux.vnet.ibm.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Minor optimization for delNetwork
......................................................................


Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Minor optimization for delNetwork
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File vdsm/configNetwork.py
Line 794:             ifdown(bonding)
oops, I did not notice it's the same.

Line 798:         nicUsers = nicOtherUsers(network, vlan, bonding, nic)
formatting, and the the removal of the test (which *is* an optimization, I just did not
understand it....)

Line 801:         if bridged and nicUsers:
it's good that you keep previous behaviour in an optimization patch. my comment is
future-facing.

--
To view, visit http://gerrit.ovirt.org/5207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I66a37cb1100411af13197642bdb13ae745bc6e53
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Gerrit-Reviewer: Livnat Peer <lpeer(a)redhat.com&gt;