Change in vdsm[master]: NetReload: netmodels for delNetwork

danken at redhat.com danken at redhat.com
Mon Jun 10 20:58:12 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: NetReload: netmodels for delNetwork
......................................................................


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

(5 inline comments)

Toni, once ready, would you verify this patch on its own right, so we can merge it in before the whole branch is ready?

Assaf, please be careful when changing the base of other people's patches - it has hidden a comment of mine. I think you could have rebased your *own* work on top of this patch, without changing this patch's base.

....................................................
File vdsm/configNetwork.py
Line 77:     topNetDev = None
Line 78:     if bonding:
Line 79:         topNetDev = Bond.objectivize(bonding, configurator, bondingOptions,
Line 80:                                      nics, mtu, _netinfo,
Line 81:                                      opts.get('implicitBonding'))
"implicitBonding" has a very confusing semantics, but hiding it in the opts kwargs of objectivizeNetwork() is not helpful - I'd rather have it declared like the other args in the docstring.
Line 82:     elif nics:
Line 83:         try:
Line 84:             nic, = nics
Line 85:         except ValueError:


....................................................
File vdsm/netconf/ifcfg.py
Line 175:         if iface.master is None:
Line 176:             self.configWriter.removeIpConfig(iface.name)
Line 177:         elif isinstance(iface.master, Bridge):
Line 178:             self.configWriter.removeBridgeMaster(iface.name)
Line 179:         multiple_usage = _netinfo.ifaceUsers(iface.name)
the name of the intermediate variable does not help me at all. If there is only a single user multiple_usage==True, which I find confusing. How about using the same name as outside the function (to_be_removed)?
Line 180:         yield not multiple_usage
Line 181: 
Line 182:         if multiple_usage or not hasattr(iface, 'destroy'):
Line 183:             ifup(iface.name)


Line 638:         cf = netinfo.NET_CONF_PREF + iface
Line 639:         for key in ('IPADDR', 'NETMASK', 'GATEWAY', 'BOOTPROTO'):
Line 640:             self._updateConfigValue(cf, key, '', True)
Line 641: 
Line 642:     def removeBridgeMaster(self, iface):
I think that a better English would be

 removeMasterBridge

as we remove the master if iface, which happens to be a bridge. We do not try to remove the master of a bridge. Or do you find my suggestion equally confusing?
Line 643:         cf = netinfo.NET_CONF_PREF + iface
Line 644:         self._updateConfigValue(cf, 'BRIDGE', '', True)
Line 645: 
Line 646:     def _getConfigValue(self, conffile, entry):


....................................................
File vdsm/netmodels.py
Line 166:         self.configurator.removeBond(self)
Line 167: 
Line 168:     @classmethod
Line 169:     def objectivize(cls, name, configurator, options, nics, mtu, _netinfo,
Line 170:                     destroy=None):
please try to rename "destroy" to something precise, such as "destroyOnMasterRemoval" (or is it??).
Line 171:         if name and nics:
Line 172:             slaves = []
Line 173:             for nic in nics:
Line 174:                 nicVlans = tuple(_netinfo.getVlansForIface(nic))


Line 195: 
Line 196:         bondObj = cls(name, configurator, slaves=slaves, options=options,
Line 197:                       mtu=mtu)
Line 198:         if destroy:
Line 199:             bondObj.destroy = True
I find adding attributes conditionally as bad style. It drops the (fake) boundary between objects and dictionaries, and forces usage of hasattr(). Can we set "destroy" to False in __init__?
Line 200:         return bondObj
Line 201: 
Line 202:     @staticmethod
Line 203:     def validateName(name):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e11fd923a4f5d07d069193278f3eadc94f327ec
Gerrit-PatchSet: 13
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>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list