Merge lp:~adeuring/launchpad/hwdb-class-udev-device-8 into lp:launchpad

Proposed by Abel Deuring
Status: Merged
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/hwdb-class-udev-device-8
Merge into: lp:launchpad
Diff against target: 197 lines
2 files modified
lib/canonical/launchpad/scripts/hwdbsubmissions.py (+1/-1)
lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py (+53/-24)
To merge this branch: bzr merge lp:~adeuring/launchpad/hwdb-class-udev-device-8
Reviewer Review Type Date Requested Status
Eleanor Berger (community) code Approve
Review via email: mp+13554@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

This branch fixes a stupid mistake I made in the property UdevDevice.scsi_controller. This property is supposed to return the node of the controller of a SCSI device.

(Background: We get data about hardware mostly from udev; a udev device is represented in the HWDB submission processing script by class UdevDevice. udev devices have parent-child relationships, where device A is an ancestor of device B, if the pathname of device B extends the pathname of device A.)

This mistake happened for two reasons:

1. I copy&pasted the sample data used in tests from "udevadm info --export-db" to the test data as defined in l/c/l/scripts/tests/test_hwdb_submission_processing.py, without looking at the data suffciently carefully. So, when reading and copying a relevant part of this data, like:

P: /devices/pci0000:00/0000:00:1f.1/host4
E: DEVTYPE=scsi_host
E: SUBSYSTEM=scsi

P: /devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4
E: SUBSYSTEM=scsi_host

P: /devices/pci0000:00/0000:00:1f.1/host4/target4:0:0
E: DEVTYPE=scsi_target
E: SUBSYSTEM=scsi

P: /devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0
E: DEVTYPE=scsi_device
E: DRIVER=sr
E: MODALIAS=scsi:t-0x05
E: SUBSYSTEM=scsi

I managed to ignore the fact that the path names /devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4 and /devices/pci0000:00/0000:00:1f.1/host4/target4:0:0 say that these nodes are siblings, but simply assumed that the former is the parent of the latter.

2. The method UdevDevice.setUo() defines lots of test data. A single udev device is represented by a dictionary like self.pci_device_data; a set of devices needed for some tests was represented by a sequence like self.scsi_device_hierarchy_data. The method UdevDevice.buildUdevDeviceHierarchy() takes such a list and simply assumed that device N of the sequence is the parent of device N+1.

This way, my wrong assumption about the parent-child relation in the example above is simply mapped into the test data.

So I changed UdevDevice.buildUdevDeviceHierarchy() to build the parent-child relations by looking at the path names of the devices (property device_id). Since the test data is copy&pasted from real-world data, we can be quite sure that the relations defined by this method are now correct.

The method now also returns a dictionary instead of a sequence, allowing to identify "interesting" devices by their path names, instead of a somewhat obscure index.

test: ./bin/test --test=test_hwdb_submission_processing

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/scripts/hwdbsubmissions.py
  lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py

== Pyflakes notices ==

lib/canonical/launchpad/scripts/hwdbsubmissions.py
    22: redefinition of unused 'etree' from line 20

== Pylint notices ==

lib/canonical/launchpad/scripts/hwdbsubmissions.py
    20: [F0401] Unable to import 'xml.etree.cElementTree' (No module named etree)

The complaint about etree is not related to my changes.

Revision history for this message
Eleanor Berger (intellectronica) wrote :

The change looks fine, r=me.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
2--- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 13:59:33 +0000
3+++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-19 09:10:30 +0000
4@@ -2665,7 +2665,7 @@
5 # While SCSI devices from valid submissions should have four
6 # ancestors, we can't be sure for bogus or broken submissions.
7 try:
8- controller = self.parent.parent.parent.parent
9+ controller = self.parent.parent.parent
10 except AttributeError:
11 controller = None
12 if controller is None:
13
14=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
15--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-15 15:28:38 +0000
16+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-19 09:10:30 +0000
17@@ -2882,8 +2882,10 @@
18 },
19 }
20
21+ self.pci_pccard_bridge_path = (
22+ '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0')
23 self.pci_pccard_bridge = {
24- 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0',
25+ 'P': self.pci_pccard_bridge_path,
26 'E': {
27 'DRIVER': 'yenta_cardbus',
28 'PCI_CLASS': '60700',
29@@ -2894,8 +2896,10 @@
30 }
31 }
32
33+ self.pccard_scsi_controller_path = (
34+ '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0')
35 self.pccard_scsi_controller_data = {
36- 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0',
37+ 'P': self.pccard_scsi_controller_path,
38 'E': {
39 'DRIVER': 'aic7xxx',
40 'PCI_CLASS': '10000',
41@@ -2937,9 +2941,11 @@
42 },
43 }
44
45+ self.scsi_scanner_device_path = (
46+ '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
47+ 'host6/target6:0:1/6:0:1:0')
48 self.scsi_scanner_device_data = {
49- 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/'
50- '0000:09:00.0/host6/target6:0:1/6:0:1:0'),
51+ 'P': self.scsi_scanner_device_path,
52 'E': {
53 'DEVTYPE': 'scsi_device',
54 'SUBSYSTEM': 'scsi',
55@@ -2997,9 +3003,10 @@
56 },
57 }
58
59+ self.ide_cdrom_device_path = (
60+ '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0')
61 self.ide_cdrom_device_data = {
62- 'P': ('/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/'
63- '4:0:0:0'),
64+ 'P': self.ide_cdrom_device_path,
65 'E': {
66 'SUBSYSTEM': 'scsi',
67 'DEVTYPE': 'scsi_device',
68@@ -3061,9 +3068,11 @@
69 },
70 }
71
72+ self.usb_storage_scsi_device_path = (
73+ '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'
74+ 'target7:0:0/7:0:0:0')
75 self.usb_storage_scsi_device = {
76- 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'
77- 'target7:0:0/7:0:0:0'),
78+ 'P': self.usb_storage_scsi_device_path,
79 'E': {
80 'DEVTYPE': 'scsi_device',
81 'DRIVER': 'sd',
82@@ -3423,22 +3432,42 @@
83 :param parser: A SubmissionParser instance to be passed to
84 the constructor of UdevDevice.
85 """
86- parent = None
87- devices = []
88+ devices = {}
89 for kwargs in device_data:
90 device = UdevDevice(parser, **kwargs)
91- devices.append(device)
92- if parent is not None:
93- parent.addChild(device)
94+ devices[device.device_id] = device
95 parent = device
96+
97+ # Build the parent-child relations so that the parent device
98+ # is that device which has the longest path matching the
99+ # start of the child's path.
100+ #
101+ # There is one exception of this rule: The root device has
102+ # the path "/devices/LNXSYSTM:00", but the paths of most of
103+ # our test deviies start with "/devices/pci". Well patch the
104+ # index temporarily in order to find children of the root
105+ # device.
106+ if '/devices/LNXSYSTM:00' in devices:
107+ devices['/devices'] = devices['/devices/LNXSYSTM:00']
108+ del devices['/devices/LNXSYSTM:00']
109+
110+ device_paths = sorted(devices, key=len, reverse=True)
111+ for path_index, path in enumerate(device_paths):
112+ for parent_path in device_paths[path_index+1:]:
113+ if path.startswith(parent_path):
114+ devices[parent_path].addChild(devices[path])
115+ break
116+ if '/devices' in devices:
117+ devices['/devices/LNXSYSTM:00'] = devices['/devices']
118+ del devices['/devices']
119 return devices
120
121 def test_scsi_controller(self):
122 """Test of UdevDevice.scsi_controller for a PCI controller."""
123 devices = self.buildUdevDeviceHierarchy(
124 self.scsi_device_hierarchy_data)
125- controller = devices[0]
126- scsi_device = devices[-1]
127+ controller = devices[self.pccard_scsi_controller_path]
128+ scsi_device = devices[self.scsi_scanner_device_path]
129 self.assertEqual(controller, scsi_device.scsi_controller)
130
131 def test_scsi_controller_insufficient_anchestors(self):
132@@ -3451,7 +3480,7 @@
133 parser.submission_key = 'UdevDevice.scsi_controller ancestor missing'
134 devices = self.buildUdevDeviceHierarchy(
135 self.scsi_device_hierarchy_data[1:], parser)
136- scsi_device = devices[-1]
137+ scsi_device = devices[self.scsi_scanner_device_path]
138 self.assertEqual(None, scsi_device.scsi_controller)
139 self.assertWarningMessage(
140 parser.submission_key,
141@@ -3471,7 +3500,7 @@
142 """Test of UdevDevice.translateScsiBus() with a real SCSI device."""
143 devices = self.buildUdevDeviceHierarchy(
144 self.scsi_device_hierarchy_data)
145- scsi_device = devices[-1]
146+ scsi_device = devices[self.scsi_scanner_device_path]
147 self.assertEqual(
148 HWBus.SCSI, scsi_device.translateScsiBus())
149
150@@ -3479,14 +3508,14 @@
151 """Test of UdevDevice.translateScsiBus() with an IDE device."""
152 devices = self.buildUdevDeviceHierarchy(
153 self.ide_device_hierarchy_data)
154- ide_device = devices[-1]
155+ ide_device = devices[self.ide_cdrom_device_path]
156 self.assertEqual(HWBus.IDE, ide_device.translateScsiBus())
157
158 def test_translateScsiBus_usb_device(self):
159 """Test of UdevDevice.translateScsiBus() with a USB device."""
160 devices = self.buildUdevDeviceHierarchy(
161 self.usb_storage_hierarchy_data)
162- usb_scsi_device = devices[-1]
163+ usb_scsi_device = devices[self.usb_storage_scsi_device_path]
164 self.assertEqual(None, usb_scsi_device.translateScsiBus())
165
166 def test_translateScsiBus_non_scsi_device(self):
167@@ -3498,8 +3527,8 @@
168 """Test of UdevDevice.translatePciBus()."""
169 devices = self.buildUdevDeviceHierarchy(
170 self.pci_bridge_pccard_hierarchy_data)
171- pci_device = devices[1]
172- pccard_device = devices[2]
173+ pci_device = devices[self.pci_pccard_bridge_path]
174+ pccard_device = devices[self.pccard_scsi_controller_path]
175 self.assertEqual(HWBus.PCI, pci_device.translatePciBus())
176 self.assertEqual(HWBus.PCCARD, pccard_device.translatePciBus())
177
178@@ -3525,8 +3554,8 @@
179 """Test of UdevDevice.real_bus for PCI devices."""
180 devices = self.buildUdevDeviceHierarchy(
181 self.pci_bridge_pccard_hierarchy_data)
182- pci_device = devices[1]
183- pccard_device = devices[2]
184+ pci_device = devices[self.pci_pccard_bridge_path]
185+ pccard_device = devices[self.pccard_scsi_controller_path]
186 self.assertEqual(HWBus.PCI, pci_device.real_bus)
187 self.assertEqual(HWBus.PCCARD, pccard_device.real_bus)
188
189@@ -3534,7 +3563,7 @@
190 """Test of UdevDevice.real_bus for a SCSI device."""
191 devices = self.buildUdevDeviceHierarchy(
192 self.scsi_device_hierarchy_data)
193- scsi_device = devices[-1]
194+ scsi_device = devices[self.scsi_scanner_device_path]
195 self.assertEqual(HWBus.SCSI, scsi_device.real_bus)
196
197 def test_real_bus_system(self):