Re: [Kimchi-devel] [PATCH 1/3] Generate libvirt's interface XML definition for vlanned bridge

Hi Aline, I would like to start a discussion about the code style for importing modules by this chance. I saw you and Rodrigo had reorganized the "import" statements(commit 65f6ad3 and e467b32). But personally, I don't agree with the rule you're following. It doesn't comply with PEP8 and bring extra unnecessary rules. 1. Currently, the kimchi imports and external imports are separated. But according to pep8: we still need differentiate the standard library and third-party library. So we just have three groups at most and put a blank line between each groups. [1] 2. For better looking, we can further organize the imports in each group: A. Sort by the import statement: all imports starting with 'import' are put together while all imports starting with 'from' are put together. But we don't need an explicit separating line between them B. Sort by module name following the first word ('import' or 'from') For this patch, I don't think we need two blank to separate them because they belongs to the same group. Does it make thanks for you?

Thanks. Mark. [1] http://www.python.org/dev/peps/pep-0008/ On 01/03/2014 02:00 AM, Aline Manera wrote: > On 01/02/2014 07:50 AM, Mark Wu wrote: >> To support vlanned virtual network, kimchi needs create the underlying >> vlan and bridge interface. Libvirt's interface driver can do it with >> a given XML definition. This patch targets to generate the XML >> according >> to the interface and vlan id. >> >> Signed-off-by: Mark Wu <wudxw(a)linux.vnet.ibm.com&gt; >> --- >> src/kimchi/networkxml.py | 17 +++++++++++++++++ >> tests/test_networkxml.py | 20 ++++++++++++++++++++ >> tests/utils.py | 6 ++++++ >> 3 files changed, 43 insertions(+) >> >> diff --git a/src/kimchi/networkxml.py b/src/kimchi/networkxml.py >> index 786cb69..25157fd 100644 >> --- a/src/kimchi/networkxml.py >> +++ b/src/kimchi/networkxml.py >> @@ -21,6 +21,8 @@ >> # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA >> 02110-1301 USA >> >> import ipaddr >> +import lxml.etree as ET > > 2 lines > >> +from lxml.builder import E >> >> >> # FIXME, do not support ipv6 >> @@ -109,3 +111,18 @@ def to_network_xml(**kwargs): >> </network> >> """ % params >> return xml >> + >> + >> +def create_vlanned_bridge_xml(bridge, interface, vlan_id): >> + vlan = E.vlan(E.interface(name=interface)) >> + vlan.set('tag', vlan_id) >> + m = E.interface( >> + E.start(mode='onboot'), >> + E.bridge( >> + E.interface( >> + vlan, >> + type='vlan', >> + name='.'.join([interface, vlan_id]))), >> + type='bridge', >> + name=bridge) >> + return ET.tostring(m) >> diff --git a/tests/test_networkxml.py b/tests/test_networkxml.py >> index 3073bce..1b2e77f 100644 >> --- a/tests/test_networkxml.py >> +++ b/tests/test_networkxml.py >> @@ -26,6 +26,7 @@ import unittest >> >> import kimchi.networkxml as nxml >> from kimchi.xmlutils import xpath_get_text >> +import utils >> >> >> class NetworkXmlTests(unittest.TestCase): >> @@ -151,3 +152,22 @@ class NetworkXmlTests(unittest.TestCase): >> netmask = xpath_get_text(xml, "/network/ip/@netmask")[0] >> self.assertEquals(netmask, >> str(ipaddr.IPNetwork(params["net"]).netmask)) >> + >> + >> +class InterfaceXmlTests(unittest.TestCase): >> + >> + def test_vlanned_bridge_no_ip(self): >> + expected_xml = """ >> + <interface type='bridge' name='br10'> >> + <start mode='onboot'/> >> + <bridge> >> + <interface type='vlan' name='em1.10'> >> + <vlan tag='10'> >> + <interface name='em1'/> >> + </vlan> >> + </interface> >> + </bridge> >> + </interface> >> + """ >> + actual_xml = nxml.create_vlanned_bridge_xml('br10', 'em1', >> '10') >> + self.assertEquals(actual_xml, >> utils.normalize_xml(expected_xml)) >> diff --git a/tests/utils.py b/tests/utils.py >> index 008f668..79fc2e2 100644 >> --- a/tests/utils.py >> +++ b/tests/utils.py >> @@ -32,6 +32,7 @@ import unittest >> >> >> from contextlib import closing >> +from lxml import etree >> >> >> import kimchi.server >> @@ -159,3 +160,8 @@ def patch_auth(): >> >> import kimchi.auth >> kimchi.auth.authenticate = _authenticate >> + >> + >> +def normalize_xml(xml_str): >> + return etree.tostring(etree.fromstring(xml_str, >> + etree.XMLParser(remove_blank_text=True))) >