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

Proposed by Sylvain Pineau
Status: Merged
Merged at revision: 1958
Proposed branch: lp:~sylvain-pineau/checkbox/bug1091633
Merge into: lp:checkbox
Diff against target: 243 lines (+81/-36)
4 files modified
checkbox/parsers/udevadm.py (+21/-0)
debian/changelog (+3/-0)
scripts/network_device_info (+56/-35)
scripts/udev_resource (+1/-1)
To merge this branch: bzr merge lp:~sylvain-pineau/checkbox/bug1091633
Reviewer Review Type Date Requested Status
Zygmunt Krynicki (community) Approve
Brendan Donegan (community) Needs Information
Review via email: mp+141054@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

@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

Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

@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

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

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.

Revision history for this message
Brendan Donegan (brendan-donegan) wrote :

Sylvain,

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

review: Needs Information
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

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

review: Approve
Revision history for this message
Zygmunt Krynicki (zyga) wrote :

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

text conflict in debian/changelog

Revision history for this message
Daniel Manrique (roadmr) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'checkbox/parsers/udevadm.py'
2--- checkbox/parsers/udevadm.py 2012-11-12 09:59:00 +0000
3+++ checkbox/parsers/udevadm.py 2012-12-21 11:10:29 +0000
4@@ -92,8 +92,15 @@
5 @property
6 def category(self):
7 if "IFINDEX" in self._environment:
8+ if "DEVTYPE" in self._environment:
9+ devtype = self._environment["DEVTYPE"]
10+ if devtype == "wlan":
11+ return "WIRELESS"
12 return "NETWORK"
13
14+ if self.bus == "sound":
15+ return "AUDIO"
16+
17 if self.bus == "ieee80211":
18 return "WIRELESS"
19
20@@ -383,6 +390,9 @@
21 if self.driver == "floppy":
22 return "Platform Device"
23
24+ if "ID_MODEL_FROM_DATABASE" in self._environment:
25+ return self._environment["ID_MODEL_FROM_DATABASE"]
26+
27 return None
28
29 @property
30@@ -407,6 +417,17 @@
31 and "ID_VENDOR_ENC" in self._environment:
32 return decode_id(self._environment["ID_VENDOR_ENC"])
33
34+ if "ID_VENDOR_FROM_DATABASE" in self._environment:
35+ return self._environment["ID_VENDOR_FROM_DATABASE"]
36+
37+ return None
38+
39+ @property
40+ def interface(self):
41+ if self.category in ("NETWORK", "WIRELESS") \
42+ and "INTERFACE" in self._environment:
43+ return self._environment["INTERFACE"]
44+
45 return None
46
47
48
49=== modified file 'debian/changelog'
50--- debian/changelog 2012-12-17 17:19:04 +0000
51+++ debian/changelog 2012-12-21 11:10:29 +0000
52@@ -47,6 +47,9 @@
53 unity_support_test and piped to ansi_parser.
54 * scripts/network_device_info: Set the driver version to 'Unknown' if the
55 modinfo_parser returns nothing (LP: #1089911)
56+ * scripts/network_device_info, scripts/udev_resource,
57+ checkbox/parsers/udevadm.py: Use udev to categorise network devices instead
58+ of lspci (LP: #1091633)
59
60 -- Daniel Manrique <roadmr@ubuntu.com> Fri, 16 Nov 2012 12:14:21 -0500
61
62
63=== modified file 'scripts/network_device_info'
64--- scripts/network_device_info 2012-12-13 13:11:40 +0000
65+++ scripts/network_device_info 2012-12-21 11:10:29 +0000
66@@ -26,6 +26,7 @@
67 import dbus
68 import sys
69 from checkbox.parsers.modinfo import ModinfoParser
70+from checkbox.parsers.udevadm import UdevadmParser
71 from subprocess import check_output, CalledProcessError, STDOUT
72
73 # This example lists basic information about network interfaces known to NM
74@@ -50,6 +51,17 @@
75 110: "Deactivating",
76 120: "Failed"}
77
78+attributes = ("category", "interface", "product", "vendor", "driver", "path")
79+
80+udev_devices = []
81+nm_devices = []
82+
83+
84+class UdevResult:
85+ def addDevice(self, device):
86+ if device.interface:
87+ udev_devices.append(device)
88+
89
90 class NetworkingDevice():
91 def __init__(self, devtype, props, dev_proxy, bus):
92@@ -68,6 +80,7 @@
93 self._driver = props['Driver']
94 except KeyError:
95 self._driver = "Unknown"
96+ self._driver_ver = "Unknown"
97
98 if self._driver != "Unknown":
99 self._modinfo = self._modinfo_parser(props['Driver'])
100@@ -87,7 +100,7 @@
101 self._state = "Unknown"
102
103 def __str__(self):
104- ret = "Type: %s\n" % self._devtype
105+ ret = "Category: %s\n" % self._devtype
106 ret += "Interface: %s\n" % self._interface
107 ret += "IP: %s\n" % self._ip
108 ret += "Driver: %s (ver: %s)\n" % (self._driver, self._driver_ver)
109@@ -124,7 +137,7 @@
110 else:
111 parser = ModinfoParser(stream)
112 modinfo = parser.get_all()
113-
114+
115 return modinfo
116
117 def _find_driver_ver(self):
118@@ -160,7 +173,8 @@
119 nm_devices = manager.GetDevices()
120 for d in nm_devices:
121 dev_proxy = bus.get_object("org.freedesktop.NetworkManager", d)
122- prop_iface = dbus.Interface(dev_proxy,"org.freedesktop.DBus.Properties")
123+ prop_iface = dbus.Interface(dev_proxy,
124+ "org.freedesktop.DBus.Properties")
125 props = prop_iface.GetAll("org.freedesktop.NetworkManager.Device")
126 try:
127 devtype = devtypes[props['DeviceType']]
128@@ -173,7 +187,7 @@
129 return devices
130
131
132-def match_counts(nm_devices, pci_devices, devtype):
133+def match_counts(nm_devices, udev_devices, devtype):
134 """
135 Ensures that the count of devices matching devtype is the same for the
136 two passed in lists, devices from Network Manager and devices from lspci.
137@@ -181,41 +195,48 @@
138 # now check that the count (by type) matches - WiFi
139 nm_type_devices = list(filter(lambda dev: dev.gettype() == devtype,
140 nm_devices))
141- pci_type_devices = list(filter(lambda dev: dev['type'] == devtype,
142- pci_devices))
143- if len(nm_type_devices) != len(pci_type_devices):
144- print("ERROR: %s devices missing: lspci showed %d devices, but "
145+ udevtype = 'WIRELESS' if devtype == 'WiFi' else 'NETWORK'
146+ udev_type_devices = list(filter(lambda dev: dev.category == udevtype,
147+ udev_devices))
148+ if len(nm_type_devices) != len(udev_type_devices):
149+ print("ERROR: %s devices missing: udev showed %d devices, but "
150 "NetworkManager saw %d devices" % (devtype,
151- len(pci_type_devices), len(nm_type_devices)))
152+ len(udev_type_devices), len(nm_type_devices)))
153 return False
154 else:
155 return True
156
157
158 def main(args):
159- pci_devices = []
160- command = "lspci"
161- for line in os.popen(command).readlines():
162- if "Ethernet controller: " in line:
163- device = {}
164- device['name'] = line.split(": ")[1].strip()
165- device['type'] = "Ethernet"
166- pci_devices.append(device)
167- elif "Network controller: " in line:
168- device = {}
169- device['name'] = line.split(": ")[1].strip()
170- device['type'] = "WiFi"
171- pci_devices.append(device)
172-
173- if pci_devices:
174- print("Devices found by lspci------------------------------")
175- for device in pci_devices:
176- print("%s: %s" % (device['type'], device['name']))
177- else:
178- print("ERROR: No Networking devices found.", file=sys.stderr)
179- return 1
180-
181- nm_devices = []
182+ try:
183+ output = check_output(['udevadm', 'info', '--export-db'])
184+ except CalledProcessError as err:
185+ raise SystemExit(err)
186+ try:
187+ output = output.decode("UTF-8")
188+ except UnicodeDecodeError as err:
189+ raise SystemExit("udevadm output is not valid UTF-8")
190+ udev = UdevadmParser(output)
191+ result = UdevResult()
192+ udev.run(result)
193+
194+ if udev_devices:
195+ print("[ Devices found by udev ]".center(80, '-'))
196+ for device in udev_devices:
197+ for attribute in attributes:
198+ value = getattr(device, attribute)
199+ if value is not None:
200+ if attribute == 'driver':
201+ props = {}
202+ props['Driver'] = value
203+ network_dev = NetworkingDevice(None, props, None, None)
204+ print("%s: %s (ver: %s)" % (attribute.capitalize(),
205+ value, network_dev._driver_ver))
206+ else:
207+ print("%s: %s" % (attribute.capitalize(), value))
208+
209+ print()
210+
211 try:
212 nm_devices = get_nm_devices()
213 except dbus.exceptions.DBusException as e:
214@@ -226,13 +247,13 @@
215 print("The Error Generated was:\n %s" % e, file=sys.stderr)
216 return 0
217
218- print("Devices found by Network Manager-------------------------")
219+ print("[ Devices found by Network Manager ]".center(80, '-'))
220 for nm_dev in nm_devices:
221 print(nm_dev)
222
223- if not match_counts(nm_devices, pci_devices, "WiFi"):
224+ if not match_counts(nm_devices, udev_devices, "WiFi"):
225 return 1
226- elif not match_counts(nm_devices, pci_devices, "Ethernet"):
227+ elif not match_counts(nm_devices, udev_devices, "Ethernet"):
228 return 1
229 else:
230 return 0
231
232=== modified file 'scripts/udev_resource'
233--- scripts/udev_resource 2012-11-12 10:09:25 +0000
234+++ scripts/udev_resource 2012-12-21 11:10:29 +0000
235@@ -27,7 +27,7 @@
236
237 attributes = ("path", "bus", "category", "driver", "product_id",
238 "vendor_id", "subproduct_id", "subvendor_id", "product",
239- "vendor",)
240+ "vendor", "interface",)
241
242 def addDevice(self, device):
243 for attribute in self.attributes:

Subscribers

People subscribed via source and target branches