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