Launchpad itself

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

  1. hwdb-class-udev-device-8
  2. Merge into devel
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
=== modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
--- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 13:59:33 +0000
+++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-19 09:10:30 +0000
@@ -2665,7 +2665,7 @@
2665 # While SCSI devices from valid submissions should have four2665 # While SCSI devices from valid submissions should have four
2666 # ancestors, we can't be sure for bogus or broken submissions.2666 # ancestors, we can't be sure for bogus or broken submissions.
2667 try:2667 try:
2668 controller = self.parent.parent.parent.parent2668 controller = self.parent.parent.parent
2669 except AttributeError:2669 except AttributeError:
2670 controller = None2670 controller = None
2671 if controller is None:2671 if controller is None:
26722672
=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-15 15:28:38 +0000
+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-19 09:10:30 +0000
@@ -2882,8 +2882,10 @@
2882 },2882 },
2883 }2883 }
28842884
2885 self.pci_pccard_bridge_path = (
2886 '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0')
2885 self.pci_pccard_bridge = {2887 self.pci_pccard_bridge = {
2886 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0',2888 'P': self.pci_pccard_bridge_path,
2887 'E': {2889 'E': {
2888 'DRIVER': 'yenta_cardbus',2890 'DRIVER': 'yenta_cardbus',
2889 'PCI_CLASS': '60700',2891 'PCI_CLASS': '60700',
@@ -2894,8 +2896,10 @@
2894 }2896 }
2895 }2897 }
28962898
2899 self.pccard_scsi_controller_path = (
2900 '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0')
2897 self.pccard_scsi_controller_data = {2901 self.pccard_scsi_controller_data = {
2898 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0',2902 'P': self.pccard_scsi_controller_path,
2899 'E': {2903 'E': {
2900 'DRIVER': 'aic7xxx',2904 'DRIVER': 'aic7xxx',
2901 'PCI_CLASS': '10000',2905 'PCI_CLASS': '10000',
@@ -2937,9 +2941,11 @@
2937 },2941 },
2938 }2942 }
29392943
2944 self.scsi_scanner_device_path = (
2945 '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/'
2946 'host6/target6:0:1/6:0:1:0')
2940 self.scsi_scanner_device_data = {2947 self.scsi_scanner_device_data = {
2941 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/'2948 'P': self.scsi_scanner_device_path,
2942 '0000:09:00.0/host6/target6:0:1/6:0:1:0'),
2943 'E': {2949 'E': {
2944 'DEVTYPE': 'scsi_device',2950 'DEVTYPE': 'scsi_device',
2945 'SUBSYSTEM': 'scsi',2951 'SUBSYSTEM': 'scsi',
@@ -2997,9 +3003,10 @@
2997 },3003 },
2998 }3004 }
29993005
3006 self.ide_cdrom_device_path = (
3007 '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0')
3000 self.ide_cdrom_device_data = {3008 self.ide_cdrom_device_data = {
3001 'P': ('/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/'3009 'P': self.ide_cdrom_device_path,
3002 '4:0:0:0'),
3003 'E': {3010 'E': {
3004 'SUBSYSTEM': 'scsi',3011 'SUBSYSTEM': 'scsi',
3005 'DEVTYPE': 'scsi_device',3012 'DEVTYPE': 'scsi_device',
@@ -3061,9 +3068,11 @@
3061 },3068 },
3062 }3069 }
30633070
3071 self.usb_storage_scsi_device_path = (
3072 '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'
3073 'target7:0:0/7:0:0:0')
3064 self.usb_storage_scsi_device = {3074 self.usb_storage_scsi_device = {
3065 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/'3075 'P': self.usb_storage_scsi_device_path,
3066 'target7:0:0/7:0:0:0'),
3067 'E': {3076 'E': {
3068 'DEVTYPE': 'scsi_device',3077 'DEVTYPE': 'scsi_device',
3069 'DRIVER': 'sd',3078 'DRIVER': 'sd',
@@ -3423,22 +3432,42 @@
3423 :param parser: A SubmissionParser instance to be passed to3432 :param parser: A SubmissionParser instance to be passed to
3424 the constructor of UdevDevice.3433 the constructor of UdevDevice.
3425 """3434 """
3426 parent = None3435 devices = {}
3427 devices = []
3428 for kwargs in device_data:3436 for kwargs in device_data:
3429 device = UdevDevice(parser, **kwargs)3437 device = UdevDevice(parser, **kwargs)
3430 devices.append(device)3438 devices[device.device_id] = device
3431 if parent is not None:
3432 parent.addChild(device)
3433 parent = device3439 parent = device
3440
3441 # Build the parent-child relations so that the parent device
3442 # is that device which has the longest path matching the
3443 # start of the child's path.
3444 #
3445 # There is one exception of this rule: The root device has
3446 # the path "/devices/LNXSYSTM:00", but the paths of most of
3447 # our test deviies start with "/devices/pci". Well patch the
3448 # index temporarily in order to find children of the root
3449 # device.
3450 if '/devices/LNXSYSTM:00' in devices:
3451 devices['/devices'] = devices['/devices/LNXSYSTM:00']
3452 del devices['/devices/LNXSYSTM:00']
3453
3454 device_paths = sorted(devices, key=len, reverse=True)
3455 for path_index, path in enumerate(device_paths):
3456 for parent_path in device_paths[path_index+1:]:
3457 if path.startswith(parent_path):
3458 devices[parent_path].addChild(devices[path])
3459 break
3460 if '/devices' in devices:
3461 devices['/devices/LNXSYSTM:00'] = devices['/devices']
3462 del devices['/devices']
3434 return devices3463 return devices
34353464
3436 def test_scsi_controller(self):3465 def test_scsi_controller(self):
3437 """Test of UdevDevice.scsi_controller for a PCI controller."""3466 """Test of UdevDevice.scsi_controller for a PCI controller."""
3438 devices = self.buildUdevDeviceHierarchy(3467 devices = self.buildUdevDeviceHierarchy(
3439 self.scsi_device_hierarchy_data)3468 self.scsi_device_hierarchy_data)
3440 controller = devices[0]3469 controller = devices[self.pccard_scsi_controller_path]
3441 scsi_device = devices[-1]3470 scsi_device = devices[self.scsi_scanner_device_path]
3442 self.assertEqual(controller, scsi_device.scsi_controller)3471 self.assertEqual(controller, scsi_device.scsi_controller)
34433472
3444 def test_scsi_controller_insufficient_anchestors(self):3473 def test_scsi_controller_insufficient_anchestors(self):
@@ -3451,7 +3480,7 @@
3451 parser.submission_key = 'UdevDevice.scsi_controller ancestor missing'3480 parser.submission_key = 'UdevDevice.scsi_controller ancestor missing'
3452 devices = self.buildUdevDeviceHierarchy(3481 devices = self.buildUdevDeviceHierarchy(
3453 self.scsi_device_hierarchy_data[1:], parser)3482 self.scsi_device_hierarchy_data[1:], parser)
3454 scsi_device = devices[-1]3483 scsi_device = devices[self.scsi_scanner_device_path]
3455 self.assertEqual(None, scsi_device.scsi_controller)3484 self.assertEqual(None, scsi_device.scsi_controller)
3456 self.assertWarningMessage(3485 self.assertWarningMessage(
3457 parser.submission_key,3486 parser.submission_key,
@@ -3471,7 +3500,7 @@
3471 """Test of UdevDevice.translateScsiBus() with a real SCSI device."""3500 """Test of UdevDevice.translateScsiBus() with a real SCSI device."""
3472 devices = self.buildUdevDeviceHierarchy(3501 devices = self.buildUdevDeviceHierarchy(
3473 self.scsi_device_hierarchy_data)3502 self.scsi_device_hierarchy_data)
3474 scsi_device = devices[-1]3503 scsi_device = devices[self.scsi_scanner_device_path]
3475 self.assertEqual(3504 self.assertEqual(
3476 HWBus.SCSI, scsi_device.translateScsiBus())3505 HWBus.SCSI, scsi_device.translateScsiBus())
34773506
@@ -3479,14 +3508,14 @@
3479 """Test of UdevDevice.translateScsiBus() with an IDE device."""3508 """Test of UdevDevice.translateScsiBus() with an IDE device."""
3480 devices = self.buildUdevDeviceHierarchy(3509 devices = self.buildUdevDeviceHierarchy(
3481 self.ide_device_hierarchy_data)3510 self.ide_device_hierarchy_data)
3482 ide_device = devices[-1]3511 ide_device = devices[self.ide_cdrom_device_path]
3483 self.assertEqual(HWBus.IDE, ide_device.translateScsiBus())3512 self.assertEqual(HWBus.IDE, ide_device.translateScsiBus())
34843513
3485 def test_translateScsiBus_usb_device(self):3514 def test_translateScsiBus_usb_device(self):
3486 """Test of UdevDevice.translateScsiBus() with a USB device."""3515 """Test of UdevDevice.translateScsiBus() with a USB device."""
3487 devices = self.buildUdevDeviceHierarchy(3516 devices = self.buildUdevDeviceHierarchy(
3488 self.usb_storage_hierarchy_data)3517 self.usb_storage_hierarchy_data)
3489 usb_scsi_device = devices[-1]3518 usb_scsi_device = devices[self.usb_storage_scsi_device_path]
3490 self.assertEqual(None, usb_scsi_device.translateScsiBus())3519 self.assertEqual(None, usb_scsi_device.translateScsiBus())
34913520
3492 def test_translateScsiBus_non_scsi_device(self):3521 def test_translateScsiBus_non_scsi_device(self):
@@ -3498,8 +3527,8 @@
3498 """Test of UdevDevice.translatePciBus()."""3527 """Test of UdevDevice.translatePciBus()."""
3499 devices = self.buildUdevDeviceHierarchy(3528 devices = self.buildUdevDeviceHierarchy(
3500 self.pci_bridge_pccard_hierarchy_data)3529 self.pci_bridge_pccard_hierarchy_data)
3501 pci_device = devices[1]3530 pci_device = devices[self.pci_pccard_bridge_path]
3502 pccard_device = devices[2]3531 pccard_device = devices[self.pccard_scsi_controller_path]
3503 self.assertEqual(HWBus.PCI, pci_device.translatePciBus())3532 self.assertEqual(HWBus.PCI, pci_device.translatePciBus())
3504 self.assertEqual(HWBus.PCCARD, pccard_device.translatePciBus())3533 self.assertEqual(HWBus.PCCARD, pccard_device.translatePciBus())
35053534
@@ -3525,8 +3554,8 @@
3525 """Test of UdevDevice.real_bus for PCI devices."""3554 """Test of UdevDevice.real_bus for PCI devices."""
3526 devices = self.buildUdevDeviceHierarchy(3555 devices = self.buildUdevDeviceHierarchy(
3527 self.pci_bridge_pccard_hierarchy_data)3556 self.pci_bridge_pccard_hierarchy_data)
3528 pci_device = devices[1]3557 pci_device = devices[self.pci_pccard_bridge_path]
3529 pccard_device = devices[2]3558 pccard_device = devices[self.pccard_scsi_controller_path]
3530 self.assertEqual(HWBus.PCI, pci_device.real_bus)3559 self.assertEqual(HWBus.PCI, pci_device.real_bus)
3531 self.assertEqual(HWBus.PCCARD, pccard_device.real_bus)3560 self.assertEqual(HWBus.PCCARD, pccard_device.real_bus)
35323561
@@ -3534,7 +3563,7 @@
3534 """Test of UdevDevice.real_bus for a SCSI device."""3563 """Test of UdevDevice.real_bus for a SCSI device."""
3535 devices = self.buildUdevDeviceHierarchy(3564 devices = self.buildUdevDeviceHierarchy(
3536 self.scsi_device_hierarchy_data)3565 self.scsi_device_hierarchy_data)
3537 scsi_device = devices[-1]3566 scsi_device = devices[self.scsi_scanner_device_path]
3538 self.assertEqual(HWBus.SCSI, scsi_device.real_bus)3567 self.assertEqual(HWBus.SCSI, scsi_device.real_bus)
35393568
3540 def test_real_bus_system(self):3569 def test_real_bus_system(self):