Merge lp:~therve/landscape-client/duplicate-network-interfaces into lp:~landscape/landscape-client/trunk

[Thomas Herve]

Triavial change, I hope, except the test which is quite scary.

[Jamu Kakar (jkakar)]

Nice work, +1!

review: Approve

[Kevin McDermott (bigkevmcd)]

Other than the ioctl response blob, which is unfortunate :-) this is great +1

(Could you maybe build up the blob using pack?)

Oh, and nice use of Mocker :-)

review: Approve

=== modified file 'landscape/lib/network.py'
--- landscape/lib/network.py	2010-06-10 08:37:55 +0000
+++ landscape/lib/network.py	2010-06-22 08:26:24 +0000
@@ -48,7 +48,7 @@
     """
     max_interfaces = 128
 
-    # setup an an array to hold our response, and initialized to null strings.
+    # Setup an an array to hold our response, and initialized to null strings.
     interfaces = array.array("B", "\0" * max_interfaces * IF_STRUCT_SIZE)
     buffer_size = interfaces.buffer_info()[0]
     packed_bytes = struct.pack(
@@ -59,11 +59,14 @@
 
     result = interfaces.tostring()
 
-    # generator over the interface names
+    # Generator over the interface names
+    already_found = set()
     for index in range(0, byte_length, IF_STRUCT_SIZE):
         ifreq_struct = result[index:index+IF_STRUCT_SIZE]
         interface_name = ifreq_struct[:ifreq_struct.index("\0")]
-        yield interface_name
+        if interface_name not in already_found:
+            already_found.add(interface_name)
+            yield interface_name
 
 
 def get_broadcast_address(sock, interface):

=== modified file 'landscape/lib/tests/test_network.py'
--- landscape/lib/tests/test_network.py	2010-06-10 08:37:55 +0000
+++ landscape/lib/tests/test_network.py	2010-06-22 08:26:24 +0000
@@ -1,9 +1,12 @@
+import array
 from cStringIO import StringIO
+import socket
 from subprocess import Popen, PIPE
 from landscape.tests.helpers import LandscapeTest
 
 from landscape.lib.network import (
-    get_network_traffic, get_active_device_info)
+    get_network_traffic, get_active_device_info, get_active_interfaces)
+from landscape.tests.mocker import ANY
 
 
 class NetworkInfoTest(LandscapeTest):
@@ -39,6 +42,57 @@
             if flags & 4096:
                 self.assertIn("MULTICAST", block)
 
+    def test_duplicate_network_interfaces(self):
+        """
+        L{get_active_interfaces} doesn't return duplicate network interfaces.
+        The call to C{fcntl.ioctl} might return the same interface several
+        times, so we make sure to clean it up.
+        """
+        import landscape.lib.network
+        original_struct_size = landscape.lib.network.IF_STRUCT_SIZE
+        landscape.lib.network.IF_STRUCT_SIZE = 40
+        self.addCleanup(
+            setattr, landscape.lib.network, "IF_STRUCT_SIZE",
+            original_struct_size)
+        # This is a fake response observed to return the same interface several
+        # times (here, br1:priv)
+        response = (
+            "lo\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02"
+            "\x00\x00\x00\x7f\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+            "\x00\x00\x00\x00\x00\x00\x00eth1:pub\x00\x00\x00\x00\x00\x00\x00"
+            "\x00\x02\x00\x00\x00\xc8\xb4\xc4.\x00\x00\x00\x00\x00\x00\x00\x00"
+            "\x00\x00\x00\x00\x00\x00\x00\x00br1:metadata\x00\x00\x00\x00\x02"
+            "\x00\x00\x00\xa9\xfe\xa9\xfe\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+            "\x00\x00\x00\x00\x00\x00\x00br1:0\x00\x00\x00\x00\x00\x00\x00\x00"
+            "\x00\x00\x00\x02\x00\x00\x00\xc9\x19\x1f\x1d\x00\x00\x00\x00\x00"
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00br1\x00\x00\x00\x00"
+            "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\xc0\xa8d"
+            "\x1d\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+            "\x00br1:priv\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\xac"
+            "\x13\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"
+            "\x00\x00\x00br1:priv\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00"
+            "\x00\xac\x13\x02A")
+
+        fake_array = array.array("B", response + "\0" * 4855)
+        mock_array = self.mocker.replace("array.array")
+        mock_array("B", ANY)
+        self.mocker.result(fake_array)
+
+        mock_ioctl = self.mocker.replace("fcntl.ioctl")
+        mock_ioctl(ANY, ANY, ANY)
+        self.mocker.result(0)
+
+        mock_unpack = self.mocker.replace("struct.unpack")
+        mock_unpack("iL", ANY)
+        self.mocker.result((280, 38643456))
+        self.mocker.replay()
+
+        sock = socket.socket(
+            socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_IP)
+        self.assertEqual(
+            ["lo", "eth1:pub", "br1:metadata", "br1:0", "br1", "br1:priv"],
+            list(get_active_interfaces(sock)))
+
     def test_get_network_traffic(self):
         """
         Network traffic is assessed via reading /proc/net/dev, verify