Merge lp:~sylvain-pineau/checkbox/bug1091633 into lp:checkbox

[Sylvain Pineau]

scripts/network_device_info: Use udev to categorise network devices instead of lspci

scripts/network_device_info: Use udev to categorise network devices instead of lspci

[Sylvain Pineau (sylvain-pineau)]

@Brendan,

As you have a system where the wifi interface is named eth1, I've set you as the first reviewer.
Please check that my udev machinery works well in such config.

Thanks

[Sylvain Pineau (sylvain-pineau)]

@Brendan,

As you have a system where the wifi interface is named eth1, I've set you as the first reviewer.
Please check that my udev machinery works well in such config.

Thanks

[Zygmunt Krynicki (zyga)]

While I don't find anything wrong with it just by looking at it I have a few general comments.

1) There are no comments in the code.

It's really hard to justify if something is correct or not. There is no rationale
explained in the code. I could look at the whole code, the history, etc and try to guess why something's
done the way it is but it's fragile and I'd rather have a code that is self explanatory to some degree.

2) There are layering issues in the coee.

For me something that's called parsers/udevadm should _not_ have any logic in it.
I'm talking about stuff like

14 + if self.bus == "sound":
15 + return "AUDIO"

This is _certainly_ something you want at a separate, higher layer. Otherwise the whole thing is ill defined
and harder to test. I'd like to see a pure parser for udevadm data along with a data layer that just
represents that and a separate mapping layer where we get our magic "WIRELESS" devices and stuff like that.

3) There are no tests being added that verify stuff works.

Writing tests is hard when things are untested and not designed to be tested easily but we just have
to get better at it. Especially for stuff like that, which is causing problems.

While I won't veto landing it we should strive to be better at those things.

[Brendan Donegan (brendan-donegan)]

Sylvain,

This has been sitting around for a while, can you respond to Zygmunts comments?

review: Needs Information

[Zygmunt Krynicki (zyga)]

Let's land this, we'll come to a point where we'll start looking at the scripts + testing architecture.

review: Approve

[Zygmunt Krynicki (zyga)]

Attempt to merge into lp:checkbox failed due to conflicts:

text conflict in debian/changelog

[Daniel Manrique (roadmr)]

Hello, I manually merged to solve the changelog conflict. Thanks!

=== modified file 'checkbox/parsers/udevadm.py'
--- checkbox/parsers/udevadm.py	2012-11-12 09:59:00 +0000
+++ checkbox/parsers/udevadm.py	2012-12-21 11:10:29 +0000
@@ -92,8 +92,15 @@
     @property
     def category(self):
         if "IFINDEX" in self._environment:
+            if "DEVTYPE" in self._environment:
+                devtype = self._environment["DEVTYPE"]
+                if devtype == "wlan":
+                    return "WIRELESS"
             return "NETWORK"
 
+        if self.bus == "sound":
+            return "AUDIO"
+
         if self.bus == "ieee80211":
             return "WIRELESS"
 
@@ -383,6 +390,9 @@
         if self.driver == "floppy":
             return "Platform Device"
 
+        if "ID_MODEL_FROM_DATABASE" in self._environment:
+            return self._environment["ID_MODEL_FROM_DATABASE"]
+
         return None
 
     @property
@@ -407,6 +417,17 @@
              and "ID_VENDOR_ENC" in self._environment:
             return decode_id(self._environment["ID_VENDOR_ENC"])
 
+        if "ID_VENDOR_FROM_DATABASE" in self._environment:
+            return self._environment["ID_VENDOR_FROM_DATABASE"]
+
+        return None
+
+    @property
+    def interface(self):
+        if self.category in ("NETWORK", "WIRELESS") \
+            and "INTERFACE" in self._environment:
+            return self._environment["INTERFACE"]
+
         return None
 
 
 
=== modified file 'debian/changelog'
--- debian/changelog	2012-12-17 17:19:04 +0000
+++ debian/changelog	2012-12-21 11:10:29 +0000
@@ -47,6 +47,9 @@
     unity_support_test and piped to ansi_parser.
   * scripts/network_device_info: Set the driver version to 'Unknown' if the
     modinfo_parser returns nothing (LP: #1089911)
+  * scripts/network_device_info, scripts/udev_resource,
+    checkbox/parsers/udevadm.py: Use udev to categorise network devices instead
+    of lspci (LP: #1091633)
 
  -- Daniel Manrique <[e-mail]>  Fri, 16 Nov 2012 12:14:21 -0500
 
 
=== modified file 'scripts/network_device_info'
--- scripts/network_device_info	2012-12-13 13:11:40 +0000
+++ scripts/network_device_info	2012-12-21 11:10:29 +0000
@@ -26,6 +26,7 @@
 import dbus
 import sys
 from checkbox.parsers.modinfo import ModinfoParser
+from checkbox.parsers.udevadm import UdevadmParser
 from subprocess import check_output, CalledProcessError, STDOUT
 
 # This example lists basic information about network interfaces known to NM
@@ -50,6 +51,17 @@
           110: "Deactivating",
           120: "Failed"}
 
+attributes = ("category", "interface", "product", "vendor", "driver", "path")
+
+udev_devices = []
+nm_devices = []
+
+
+class UdevResult:
+    def addDevice(self, device):
+        if device.interface:
+            udev_devices.append(device)
+
 
 class NetworkingDevice():
     def __init__(self, devtype, props, dev_proxy, bus):
@@ -68,6 +80,7 @@
             self._driver = props['Driver']
         except KeyError:
             self._driver = "Unknown"
+            self._driver_ver = "Unknown"
 
         if self._driver != "Unknown":
             self._modinfo = self._modinfo_parser(props['Driver'])
@@ -87,7 +100,7 @@
             self._state = "Unknown"
 
     def __str__(self):
-        ret = "Type: %s\n" % self._devtype
+        ret = "Category: %s\n" % self._devtype
         ret += "Interface: %s\n" % self._interface
         ret += "IP: %s\n" % self._ip
         ret += "Driver: %s (ver: %s)\n" % (self._driver, self._driver_ver)
@@ -124,7 +137,7 @@
         else:
             parser = ModinfoParser(stream)
             modinfo = parser.get_all()
-        
+
         return modinfo
 
     def _find_driver_ver(self):
@@ -160,7 +173,8 @@
     nm_devices = manager.GetDevices()
     for d in nm_devices:
         dev_proxy = bus.get_object("org.freedesktop.NetworkManager", d)
-        prop_iface = dbus.Interface(dev_proxy,"org.freedesktop.DBus.Properties")
+        prop_iface = dbus.Interface(dev_proxy,
+                                    "org.freedesktop.DBus.Properties")
         props = prop_iface.GetAll("org.freedesktop.NetworkManager.Device")
         try:
             devtype = devtypes[props['DeviceType']]
@@ -173,7 +187,7 @@
     return devices
 
 
-def match_counts(nm_devices, pci_devices, devtype):
+def match_counts(nm_devices, udev_devices, devtype):
     """
     Ensures that the count of devices matching devtype is the same for the
     two passed in lists, devices from Network Manager and devices from lspci.
@@ -181,41 +195,48 @@
     # now check that the count (by type) matches - WiFi
     nm_type_devices = list(filter(lambda dev: dev.gettype() == devtype,
         nm_devices))
-    pci_type_devices = list(filter(lambda dev: dev['type'] == devtype,
-        pci_devices))
-    if len(nm_type_devices) != len(pci_type_devices):
-        print("ERROR: %s devices missing: lspci showed %d devices, but "
+    udevtype = 'WIRELESS' if devtype == 'WiFi' else 'NETWORK'
+    udev_type_devices = list(filter(lambda dev: dev.category == udevtype,
+        udev_devices))
+    if len(nm_type_devices) != len(udev_type_devices):
+        print("ERROR: %s devices missing: udev showed %d devices, but "
               "NetworkManager saw %d devices" % (devtype,
-              len(pci_type_devices), len(nm_type_devices)))
+              len(udev_type_devices), len(nm_type_devices)))
         return False
     else:
         return True
 
 
 def main(args):
-    pci_devices = []
-    command = "lspci"
-    for line in os.popen(command).readlines():
-        if "Ethernet controller: " in line:
-            device = {}
-            device['name'] = line.split(": ")[1].strip()
-            device['type'] = "Ethernet"
-            pci_devices.append(device)
-        elif "Network controller: " in line:
-            device = {}
-            device['name'] = line.split(": ")[1].strip()
-            device['type'] = "WiFi"
-            pci_devices.append(device)
-
-    if pci_devices:
-        print("Devices found by lspci------------------------------")
-        for device in pci_devices:
-            print("%s: %s" % (device['type'], device['name']))
-    else:
-        print("ERROR: No Networking devices found.", file=sys.stderr)
-        return 1
-
-    nm_devices = []
+    try:
+        output = check_output(['udevadm', 'info', '--export-db'])
+    except CalledProcessError as err:
+        raise SystemExit(err)
+    try:
+        output = output.decode("UTF-8")
+    except UnicodeDecodeError as err:
+        raise SystemExit("udevadm output is not valid UTF-8")
+    udev = UdevadmParser(output)
+    result = UdevResult()
+    udev.run(result)
+
+    if udev_devices:
+        print("[ Devices found by udev ]".center(80, '-'))
+        for device in udev_devices:
+            for attribute in attributes:
+                value = getattr(device, attribute)
+                if value is not None:
+                    if attribute == 'driver':
+                        props = {}
+                        props['Driver'] = value
+                        network_dev = NetworkingDevice(None, props, None, None)
+                        print("%s: %s (ver: %s)" % (attribute.capitalize(),
+                              value, network_dev._driver_ver))
+                    else:
+                        print("%s: %s" % (attribute.capitalize(), value))
+
+            print()
+
     try:
         nm_devices = get_nm_devices()
     except dbus.exceptions.DBusException as e:
@@ -226,13 +247,13 @@
         print("The Error Generated was:\n %s" % e, file=sys.stderr)
         return 0
 
-    print("Devices found by Network Manager-------------------------")
+    print("[ Devices found by Network Manager ]".center(80, '-'))
     for nm_dev in nm_devices:
         print(nm_dev)
 
-    if not match_counts(nm_devices, pci_devices, "WiFi"):
+    if not match_counts(nm_devices, udev_devices, "WiFi"):
         return 1
-    elif not match_counts(nm_devices, pci_devices, "Ethernet"):
+    elif not match_counts(nm_devices, udev_devices, "Ethernet"):
         return 1
     else:
         return 0
 
=== modified file 'scripts/udev_resource'
--- scripts/udev_resource	2012-11-12 10:09:25 +0000
+++ scripts/udev_resource	2012-12-21 11:10:29 +0000
@@ -27,7 +27,7 @@
 
     attributes = ("path", "bus", "category", "driver", "product_id",
                   "vendor_id", "subproduct_id", "subvendor_id", "product",
-                  "vendor",)
+                  "vendor", "interface",)
 
     def addDevice(self, device):
         for attribute in self.attributes: