Antoni Segura Puimedon has posted comments on this change.

Change subject: NetReload: netmodels for addNetwork
......................................................................


Patch Set 7: (6 inline comments)

....................................................
File vdsm/configNetwork.py
Line 47: MAX_BRIDGE_NAME_LEN = 15
Line 48: ILLEGAL_BRIDGE_CHARS = frozenset(':. \t')
Line 49: 
Line 50: 
Line 51: def objectivizeNetwork(bridge=None, vlan=None, bonding=None,
This method takes a few arguments that describe a network and makes an object 
oriented representation out of it. It does not create a network and I am not a 
fan of unnecessary long names like "makeObjectOrientedNetworkRepresentation".

Good point about the docstring.
Line 52:                        bondingOptions=None, nics=None, mtu=None, 
ipaddr=None,
Line 53:                        netmask=None, gateway=None, bootproto=None,
Line 54:                        _netinfo=None, configurator=None, **opts):
Line 55:     if configurator is None:


Line 76:     if bridge:
Line 77:         topObj = Bridge(bridge, configurator, ports=[topObj] if topObj 
else [],
Line 78:                         mtu=mtu, stp=opts.get('stp'),
Line 79:                         forwardDelay=opts.get('forward_delay', 0))
Line 80:     topObj.ip = IpConfig(inet=IPv4(ipaddr, netmask, gateway),
AFAIK This code can't be reached by calls from vdsClient nor the engine without 
a topObj being set. I could raise a  BAD_PARAMS error I guess...
Line 81:                          bootproto=bootproto)
Line 82:     return topObj
Line 83: 
Line 84: 


Line 81:                          bootproto=bootproto)
Line 82:     return topObj
Line 83: 
Line 84: 
Line 85: def _validateInterNetworkCompatibility(ni, vlan, iface, bridged):
I'd leave this for a follow-up patch.
Line 86:     """
Line 87:     Verify network compatibility with other networks on iface 
(bond/nic).
Line 88: 
Line 89:     Only following combinations allowed:


Line 175:     if bonding not in vlanedIfaces:
Line 176:         for nic in nics:
Line 177:             if nic not in vlanedIfaces:
Line 178:                 ifdown(nic)
Line 179:     netEnt.configure(network=network, **options)
I'm afraid I don't understand this. Would you please rephrase?
Line 180: 
Line 181: 
Line 182: def assertBridgeClean(bridge, vlan, bonding, nics):
Line 183:     ports = set(netinfo.ports(bridge))


....................................................
File vdsm/netmodels.py
Line 27: from vdsm import netinfo
Line 28: import neterrors as ne
Line 29: 
Line 30: 
Line 31: class NetDevice(object):
Done
Line 32:     def __init__(self, name, configurator, ipconfig=None, mtu=None):
Line 33:         self.name = name
Line 34:         self.ip = ipconfig
Line 35:         self.mtu = mtu


Line 114:     '''This class represents traditional kernel bridges.'''
Line 115:     MAX_BRIDGE_NAME_LEN = 15
Line 116:     ILLEGAL_BRIDGE_CHARS = frozenset(':. \t')
Line 117: 
Line 118:     def __init__(self, name, configurator, ipconfig=None, mtu=None, 
ports=(),
You are right that we don't do it at the moment and there are no plans to do 
it. Thus, this premature extensibility point (it was here for netinfo) will be 
dropped.
Line 119:                  forwardDelay=0, stp=None):
Line 120:         self.validateName(name)
Line 121:         self.ports = ports
Line 122:         self.forwardDelay = forwardDelay


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba5c5b84760e27245cbe34c3b290c54e51278e72
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com>
Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to