Change in vdsm[master]: Skip setting up network if no change is found.

asegurap at redhat.com asegurap at redhat.com
Mon Sep 17 09:11:30 UTC 2012


Antoni Segura Puimedon has posted comments on this change.

Change subject: Skip setting up network if no change is found.
......................................................................


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

(4 inline comments)

Just a couple of issues, but I like the spirit of the patch.

....................................................
File vdsm/configNetwork.py
Line 1141:             configWriter.restoreBackups()
Line 1142:             return define.errCode['noConPeer']['status']['code']
Line 1143: 
Line 1144: 
Line 1145: class NetworksOptions():
Could we have NetworkDefinitions instead?
IMHO it makes it sound clearer if we replace options for definitions throughout the patch.
Line 1146: 
Line 1147:     def __init__(self):
Line 1148:         self.curOptions = {}
Line 1149:         self.oldOptions = {}


Line 1168:         self.curOptions = self.oldOptions
Line 1169:         self.oldOptions = {}
Line 1170: 
Line 1171: 
Line 1172: networksOptions = NetworksOptions()
I would lead this variable with an underscore to indicate that it is an implementation detail, not to be relied upon externally.
Line 1173: 
Line 1174: 
Line 1175: def _validateNetworkSetup(networks={}, bondings={}):
Line 1176:     _netinfo = netinfo.NetInfo()


Line 1270: 
Line 1271:             del bondings[bond]
Line 1272: 
Line 1273: 
Line 1274: networksParams = {}
I can't see where this is used.
Line 1275: 
Line 1276: 
Line 1277: def setupNetworks(networks={}, bondings={}, **options):
Line 1278:     """Add/Edit/Remove configuration for networks and bondings.


Line 1330: 
Line 1331:         force = options.get('force', False)
Line 1332:         if not utils.tobool(force):
Line 1333:             logging.debug("Validating configuration")
Line 1334:             _validateNetworkSetup(networks, bondings)
According to the method docstring this is right. If we are sure nobody passes networks nor bondings as lists of tuples, I am enthusiastically for this change.
Line 1335: 
Line 1336:         logger.debug("Applying...")
Line 1337:         try:
Line 1338:             # Remove edited networks and networks with 'remove' attribute


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I364b2edcd9e44112dcab8f2f27dbbbaa74224093
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>


More information about the vdsm-patches mailing list