On 01/03/2014 10:34 AM, Mark Wu wrote:
On 01/03/2014 02:04 AM, Aline Manera wrote:
> On 01/02/2014 07:50 AM, Mark Wu wrote:
>> It creates a vlan interface on top of the given nic or bond interface,
>> and bridge the vlan interface to a new created bridge, which is
used to
>> forward VM's traffic. So all packets transmitted from VM will be
tagged
>> before it departs from the physical network interface.
>>
>> Signed-off-by: Mark Wu <wudxw(a)linux.vnet.ibm.com>
>> ---
>> src/kimchi/API.json | 6 ++++++
>> src/kimchi/model.py | 47
++++++++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 50 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/kimchi/API.json b/src/kimchi/API.json
>> index 7b90826..07bf41e 100644
>> --- a/src/kimchi/API.json
>> +++ b/src/kimchi/API.json
>> @@ -56,6 +56,12 @@
>> "interface": {
>> "description": "The name of a network interface on the
host",
>> "type": "string"
>> + },
>> + "vlan_id": {
>> + "description": "Network's VLAN ID",
>> + "type": "integer",
>> + "maximum": 4094,
>> + "minimum": 1
>> }
>> }
>> },
>> diff --git a/src/kimchi/model.py b/src/kimchi/model.py
>> index a6790b8..b3589e9 100644
>> --- a/src/kimchi/model.py
>> +++ b/src/kimchi/model.py
>> @@ -58,6 +58,7 @@ from kimchi import config
>> from kimchi import isoinfo
>> from kimchi import netinfo
>> from kimchi import network as knetwork
>> +from kimchi import networkxml
>> from kimchi import vnc
>> from kimchi import xmlutils
>> from kimchi.asynctask import AsyncTask
>> @@ -65,7 +66,6 @@ from kimchi.distroloader import DistroLoader
>> from kimchi.exception import InvalidOperation, InvalidParameter,
MissingParameter
>> from kimchi.exception import NotFoundError, OperationFailed
>> from kimchi.featuretests import FeatureTests
>> -from kimchi.networkxml import to_network_xml
>> from kimchi.objectstore import ObjectStore
>> from kimchi.scan import Scanner
>> from kimchi.screenshot import VMScreenshot
>> @@ -806,7 +806,12 @@ class Model(object):
>> if netinfo.is_bridge(iface):
>> params['bridge'] = iface
>> elif netinfo.is_bare_nic(iface) or netinfo.is_bonding(iface):
>> - params['forward']['dev'] = iface
>> + if params.get('vlan_id') is None:
>> + params['forward']['dev'] = iface
>> + else:
>> + params['bridge'] = \
>> + self._create_vlanned_bridge(str(iface),
>> + str(params['vlan_id']))
>> else:
>> raise InvalidParameter("the interface should be bare nic, "
>> "bonding or bridge device.")
>> @@ -830,7 +835,7 @@ class Model(object):
>> if connection == 'bridge':
>> self._set_network_bridge(params)
>>
>> - xml = to_network_xml(**params)
>> + xml = networkxml.to_network_xml(**params)
>>
>> try:
>> network = conn.networkDefineXML(xml)
>> @@ -888,8 +893,44 @@ class Model(object):
>> if network.isActive():
>> raise InvalidOperation(
>> "Unable to delete the active network %s" % name)
>> + self._remove_vlanned_bridge(network)
>> network.undefine()
>>
>> + def _get_vlanned_bridge_name(self, interface, vlan_id):
>> + return '-'.join(('kimchi', interface, vlan_id))
>> +
>> + def _is_vlanned_bridge(self, bridge):
>> + return bridge.startswith('kimchi-')
>> +
>> + def _create_vlanned_bridge(self, interface, vlan_id):
>> + bridge = self._get_vlanned_bridge_name(interface, vlan_id)
>> + bridge_xml = networkxml.create_vlanned_bridge_xml(bridge, interface,
>> + vlan_id)
>> + conn = self.conn.get()
>> + conn.changeBegin()
>> + try:
>> + vlanned_bridge = conn.interfaceDefineXML(bridge_xml)
>> + vlanned_bridge.create()
>> + except:
>> + conn.changeRollback()
>> + raise OperationFailed('Failed to create vlanned bridge')
>> + else:
>> + conn.changeCommit()
>> + return bridge
>> +
>
> You can avoid multiple indents by removing 'else' statement
I prefer to use 'try...except...else' to because it matches
'do...rollback...commit' clearly.
If the 'else' statement is removed, we need to check the code in
'except' to see if unindent code
only runs on success. Moreover, the statement under 'else' is very
short. We don't need
care about the indents. Does it make sense?
>> + def _remove_vlanned_bridge(self, network):
>> + try:
>> + bridge = network.bridgeName()
>> + except libvirt.libvirtError:
>> + pass
>
> Log the information.
>
> kimchi_log.info() or kimchi_log.debug()
It doesn't mean any actual error. It only happen when the network
doesn't have a bridge, like the networks
forwarded by macvtap. So it's just like a normal 'if' statement to
check the forward mode of the network.
I don't think we need a special logging here.
sure, some type networks do not
has bridge, and libvirt will throw a
exception.
>> + else:
>> + if self._is_vlanned_bridge(bridge):
>> + conn = self.conn.get()
>> + iface = conn.interfaceLookupByName(bridge)
>> + if iface.isActive():
>> + iface.destroy()
>> + iface.undefine()
>
> You can avoid multiple indents by removing 'else' statement
Nope, we can't remove 'else' here. It change the semantic. Because the
'pass' statement in
'except' block doesn't stop execution, it will run into the code no
matter it succeeds or not.
We need only call it on success.
>> +
>> def add_task(self, target_uri, fn, opaque=None):
>> id = self.next_taskid
>> self.next_taskid = self.next_taskid + 1
--
Thanks and best regards!
Sheldon Feng(冯少合)<shaohef(a)linux.vnet.ibm.com>
IBM Linux Technology Center