Change in vdsm[master]: Make vdsm/configNetwork.py PEP8 clean
zhshzhou at linux.vnet.ibm.com
zhshzhou at linux.vnet.ibm.com
Thu Sep 6 05:58:45 UTC 2012
Zhou Zheng Sheng has posted comments on this change.
Change subject: Make vdsm/configNetwork.py PEP8 clean
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(16 inline comments)
....................................................
File vdsm/configNetwork.py
Line 437: gateway=None, bootproto=None, delay='0', onboot='yes', **kwargs):
Line 438: """ Create ifcfg-* file with proper fields for bridge """
Line 439: conf = 'TYPE=Bridge\nDELAY=%s\n' % pipes.quote(delay)
Line 440: self._createConfFile(conf, name, ipaddr, netmask, gateway,
Line 441: bootproto, mtu, onboot, **kwargs)
Seems too much indent. Is it better we let "bootproto" align with "conf"?
Line 442:
Line 443: def addVlan(self, vlanId, iface, network, mtu=None, bridged=True,
Line 444: ipaddr=None, netmask=None, gateway=None, bootproto=None,
Line 445: onboot='yes', **kwargs):
Line 579: the configuration file
Line 580: """
Line 581: with open(conffile) as f:
Line 582: entries = [line for line in f.readlines()
Line 583: if not line.startswith(entry + '=')]
Maybe "if" can align with "line" in the upper line.
Line 584:
Line 585: if not delete:
Line 586: entries.append('\n' + entry + '=' + value)
Line 587:
Line 671:
Line 672: def isBridgeNameValid(bridgeName):
Line 673: return bridgeName and len(bridgeName) <= MAX_BRIDGE_NAME_LEN and \
Line 674: len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and \
Line 675: not bridgeName.startswith('-')
Seems parenthesis can be used here.
return (bridgeName and len(bridgeName) <= MAX_BRIDGE_NAME_LEN and
len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and
not bridgeName.startswith('-'))
Line 676:
Line 677:
Line 678: def validateBridgeName(bridgeName):
Line 679: if not isBridgeNameValid(bridgeName):
Line 724: try:
Line 725: for option in bondingOptions.split():
Line 726: key, value = option.split('=')
Line 727: if not os.path.exists('/sys/class/net/%(bonding)s/bonding/%(key)s'
Line 728: % locals()):
How about
if not os.path.exists(
'/sys/class/net/%(bonding)s/bonding/%(key)s' % locals()):
Line 729: raise ConfigNetworkError(ne.ERR_BAD_BONDING,
Line 730: "%r is not a valid bonding option" % key)
Line 731: except ValueError:
Line 732: raise ConfigNetworkError(ne.ERR_BAD_BONDING,
Line 737: try:
Line 738: if not 0 <= int(vlan) <= MAX_VLAN_ID:
Line 739: raise ConfigNetworkError(ne.ERR_BAD_VLAN,
Line 740: 'vlan id out of range: %r, must be 0..%s'
Line 741: % (vlan, MAX_VLAN_ID))
Usually the line breaks after a binary operation like '%'.
Can I suggest:
raise ConfigNetworkError(
ne.ERR_BAD_VLAN, 'vlan id out of range: %r, must be 0..%s' %
(vlan, MAX_VLAN_ID))
Line 742: except ValueError:
Line 743: raise ConfigNetworkError(ne.ERR_BAD_VLAN, 'vlan id must be a number')
Line 744:
Line 745:
Line 757: # is a non-VLANed network over our iface
Line 758: for (iface_net, iface_vlan) in ifaces:
Line 759: if iface_vlan is None:
Line 760: raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
Line 761: "interface %r already member of network %r" % \
This backslash is redundant.
Line 762: (iface, iface_net))
Line 763:
Line 764: ifaces_bridgeless = tuple(ni.getBridgelessNetworksAndVlansForIface(iface))
Line 765: ifaces_bridged = tuple(ni.getBridgedNetworksAndVlansForIface(iface))
Line 779: # Want to add non-VLANed bridged network,
Line 780: # check whether interface is empty
Line 781: elif ifaces_bridged or ifaces_bridgeless:
Line 782: raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
Line 783: "interface %r already has networks" % \
This backslash is redundant.
Line 784: (iface))
Line 785:
Line 786:
Line 787: def _addNetworkValidation(_netinfo, network, vlan, bonding, nics, ipaddr,
Line 844: # still relevant if bonding is None
Line 845: bondingForNics = _netinfo.getBondingForNic(nic)
Line 846: if bondingForNics and bondingForNics != bonding:
Line 847: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 848: "nic %s already enslaved to %s" % \
This backslash is redundant.
Line 849: (nic, bondingForNics))
Line 850:
Line 851: # Make sure nics don't used by vlans if bond requested
Line 852: if bonding:
Line 852: if bonding:
Line 853: vlansForNic = tuple(_netinfo.getVlansForNic(nic))
Line 854: if vlansForNic:
Line 855: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 856: "nic %s already used by vlans %s" % \
This backslash is redundant.
Line 857: (nic, vlansForNic))
Line 858: networksForNic = tuple(_netinfo.getNetworksForIface(nic))
Line 859: if networksForNic:
Line 860: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 857: (nic, vlansForNic))
Line 858: networksForNic = tuple(_netinfo.getNetworksForIface(nic))
Line 859: if networksForNic:
Line 860: raise ConfigNetworkError(ne.ERR_USED_NIC,
Line 861: "nic %s already used by networks %s" % \
This backslash is redundant.
Line 862: (nic, networksForNic))
Line 863: else:
Line 864: _validateInterNetworkCompatibility(_netinfo, vlan, nic, bridged)
Line 865:
Line 906: ifdown(nic)
Line 907:
Line 908: if bridged:
Line 909: configWriter.addBridge(network, ipaddr=ipaddr, netmask=netmask,
Line 910: mtu=mtu, gateway=gateway, **options)
Could "mtu" align with "network"?
Line 911: ifdown(network)
Line 912: # We need to define (if requested) ip, mask & gateway on ifcfg-*
Line 913: # only on most top device according to following order:
Line 914: # bridge -> vlan -> bond -> nic
Line 940: configWriter.addBonding(bonding, bridge=bridgeForNic,
Line 941: bondingOptions=bondingOptions,
Line 942: mtu=max(prevmtu, mtu),
Line 943: ipaddr=ipaddr, netmask=netmask,
Line 944: gateway=gateway, **options)
Can it be
configWriter.addBonding(bonding, bridge=bridgeForNic,
bondingOptions=bondingOptions,
mtu=max(prevmtu, mtu),
ipaddr=ipaddr, netmask=netmask,
gateway=gateway, **options)
Line 945: # reset ip, netmask, gateway for lower level devices
Line 946: ipaddr = netmask = gateway = None
Line 947:
Line 948: for nic in nics:
Line 1066: if bonding:
Line 1067: validateBondingName(bonding)
Line 1068: if set(nics) != set(_netinfo.bondings[bonding]["slaves"]):
Line 1069: raise ConfigNetworkError(ne.ERR_BAD_NIC,
Line 1070: "delNetwork: %s are not all nics enslaved to %s" % \
This backslash is redundant.
Line 1071: (nics, bonding))
Line 1072: if vlan:
Line 1073: validateVlanId(vlan)
Line 1074: if bridged:
Line 1144: configWriter.restoreBackups()
Line 1145: raise
Line 1146: if utils.tobool(options.get('connectivityCheck', False)):
Line 1147: if not clientSeen(int(options.get('connectivityTimeout',
Line 1148: CONNECTIVITY_TIMEOUT_DEFAULT))):
Is it better
if not clientSeen(int(options.get('connectivityTimeout',
CONNECTIVITY_TIMEOUT_DEFAULT))):
Line 1149: delNetwork(newBridge, force=True)
Line 1150: configWriter.restoreBackups()
Line 1151: return define.errCode['noConPeer']['status']['code']
Line 1152:
Line 1187: _netinfo = netinfo.NetInfo()
Line 1188:
Line 1189: for bond, bondAttrs in bondings.iteritems():
Line 1190: logger.debug("Creating/Editing bond %s with attributes %s",
Line 1191: bond, bondAttrs)
Can "bond" align with "Creating" ?
Line 1192:
Line 1193: brNets = list(_netinfo.getBridgedNetworksForIface(bond))
Line 1194: # Only one bridged-non-VLANed network allowed on same nic/bond
Line 1195: bridge = brNets[0] if brNets else None
Line 1346: elif d['bonding'] in _ni.bondings:
Line 1347: logger.debug("Updating bond %r info", d['bonding'])
Line 1348: d['nics'] = _ni.bondings[d['bonding']]['slaves']
Line 1349: d['bondingOptions'] = (_ni.bondings[d['bonding']]
Line 1350: ['cfg'].get('BONDING_OPTS', None))
How about
d['bondingOptions'] = \
_ni.bondings[d['bonding']]['cfg'].get(
'BONDING_OPTS', None))
Line 1351: else:
Line 1352: d['nics'] = [d.pop('nic')]
Line 1353: d['force'] = force
Line 1354:
--
To view, visit http://gerrit.ovirt.org/7758
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02be4c0cdb1868a8518a73e22dfb77adedc08f3f
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
More information about the vdsm-patches
mailing list