Dan Kenigsberg has posted comments on this change.
Change subject: WIP: refactor configNetwork
......................................................................
Patch Set 1: (3 inline comments)
....................................................
File vdsm/configNetwork.py
Line 837: if brifs:
Line 838: raise ConfigNetworkError(ne.ERR_USED_BRIDGE, 'bridge %s has
interfaces %s connected' % (bridge, brifs))
Line 839:
Line 840:
Line 841: class Configurator(object):
I'm not sure that Python likes these kind of "pure virtual methods". Their only
justification is documentation, which can be achieved well enough using a
docstring.
Line 842:
Line 843: def addNetwork(self, network, vlan=None, bonding=None, nics=None,
Line 844: ipaddr=None, netmask=None, mtu=None, gateway=None,
Line 845: force=False, bondingOptions=None, bridged=True,
**options):
Line 855:
Line 856: class NativeConfigurator(Configurator):
Line 857:
Line 858: def __init__(self):
Line 859: self.confiWriter = ConfigWriter()
you have a singleton NativeConfigurator with a single confiWriter (typo?). this
breaks the intention of having an unrelated rollback for each verb call. True,
network verbs are mutually exclusive anyway, so there is only one ConfigWriter
already. But that's an artifact, not a structural constraint.
Line 860:
Line 861: def addNetwork(self, network, vlan=None, bonding=None, nics=None,
Line 862: ipaddr=None, netmask=None, mtu=None, gateway=None,
Line 863: force=False, bondingOptions=None, bridged=True,
**options):
Line 1102: # Anyway, we will not be able to take it down with
connected VMs
Line 1103:
Line 1104: # First we need to prepare all conf files
Line 1105: self.configWriter.addBonding(bond, bridge=bridge,
mtu=mtu,
Line 1106:
bondingOptions=bondAttrs.get('options', None))
Since you are moving so much code around - would you agree to first do a pep8
cleanup? Inadvertently, you add pep8 errors here.
Line 1107:
Line 1108: for nic in bondAttrs['nics']:
Line 1109: self.configWriter.addNic(nic, bonding=bond, mtu=mtu)
Line 1110:
--
To view, visit http://gerrit.ovirt.org/7714
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I66710fdf8fd73beb06124f72ef9857df7393a602
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
