Hi Michael, thanks for your review! On 15.10.2009 13:54, Michael Nelson wrote: > Review: Approve code >> This branch adds a property scsi_controller to class UdevDevice in the script >> hwdbsubmissons.py. > > Hi Abel. Again, thanks for the details about this branch - it's very helpful to have such a clear explanation when the topic is quite foreign (at least, to me!). > > I didn't have anything other than a few questions (see below). Great work! > > [snip] > >> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py' >> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-13 21:03:31 +0000 >> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 10:53:08 +0000 > [snip] >> @@ -2617,6 +2635,29 @@ >> """See `BaseDevice`.""" >> return self.getVendorOrProductID('product') >> >> + @property >> + def driver_name(self): >> + """See `BaseDevice`.""" >> + return self.udev['E'].get('DRIVER') >> + >> + @property >> + def scsi_controller(self): >> + """See `BaseDevice`.""" >> + if self.raw_bus != 'scsi_device': >> + return None >> + >> + current_device = self >> + for ancestor_level in range(1, 5): >> + if current_device.parent is None: >> + # While SCSI devices from valid submissions should have a >> + # parent and a grandparent, we can't be sure for bogus or >> + # broken submissions. >> + self.parser._logWarning( >> + 'Found a SCSI device without a sufficient number of ' >> + 'ancestors: %s' % self.device_id) >> + return None >> + current_device = current_device.parent >> + return current_device > > Nice! > > Actually, if you're just presenting a general log warning, I wonder whether > doing something like the following would be more readable? > try: > controller = self.parent.parent.parent.parent > except AttributeError: > return None > > Up to you. Right, that's a bit simpler and beter eadable. Changed. > >> >> class ProcessingLoop(object): >> """An `ITunableLoop` for processing HWDB submissions.""" >> >> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py' >> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-13 21:02:35 +0000 >> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-15 10:53:08 +0000 >> @@ -539,6 +539,214 @@ >> 'HAL property info.bus.') >> >> >> + def test_HALDevice_scsi_controller_usb_storage_device(self): >> + """test of HALDevice.scsi_controller. >> + >> + The physical device is a USB storage device. >> + """ >> + devices = [ >> + # The main node of the USB storage device. >> + { >> + 'id': 1, >> + 'udi': self.UDI_USB_STORAGE, >> + 'properties': { >> + 'info.bus': ('usb_device', 'str'), >> + }, >> + }, > > Just a style question (the answer to which I'm not certain), based on: > > https://dev.launchpad.net/PythonStyleGuide#Multiline%20braces > > should this be: > > devices = [{ > # The main node of the USB storage device. > 'id': 1, > 'udi': self.UDI_USB_STORAGE, > 'properties': { > 'info.bus': ('usb_device', 'str'), > }, > }, { > ... > > hrm, probably not. Yes, afaics, the way you've done it is the only way > that fits both style rules on that page - it just looks strange with the > opening brace being indented (which doesn't happen for non-nested braces). > Hmm, maybe we should have a nested example in the styleguide. > > So, nothing to change here, move along please! :) These excessively long definitions of sometimes nested dictionaries and lists are a littls nightmare... But it is hard to avoid them. > >> + # The storage interface of the USB device. >> + { >> + 'id': 2, >> + 'udi': self.UDI_USB_STORAGE_IF0, >> + 'properties': { >> + 'info.bus': ('usb', 'str'), >> + 'info.parent': (self.UDI_USB_STORAGE, 'str'), >> + }, >> + }, >> + # The fake SCSI host of the storage device. Note that HAL does >> + # _not_ provide the info.bus property. >> + { >> + 'id': 3, >> + 'udi': self.UDI_USB_STORAGE_SCSI_HOST, >> + 'properties': { >> + 'info.parent': (self.UDI_USB_STORAGE_IF0, 'str'), >> + }, >> + }, >> + # The fake SCSI disk. >> + { >> + 'id': 3, >> + 'udi': self.UDI_USB_STORAGE_SCSI_DEVICE, >> + 'properties': { >> + 'info.bus': ('scsi', 'str'), >> + 'info.parent': (self.UDI_USB_STORAGE_SCSI_HOST, 'str'), >> + }, >> + }, >> + ] >> + parsed_data = { >> + 'hardware': { >> + 'hal': { >> + 'devices': devices, >> + }, >> + }, >> + } >> + >> + parser = SubmissionParser() >> + parser.buildDeviceList(parsed_data) >> + >> + usb_fake_scsi_disk = parser.hal_devices[ >> + self.UDI_USB_STORAGE_SCSI_DEVICE] >> + usb_main_device = parser.hal_devices[self.UDI_USB_STORAGE_IF0] >> + self.assertEqual(usb_main_device, usb_fake_scsi_disk.scsi_controller) >> + >> + def test_HALDevice_scsi_controller_pci_controller(self): >> + """test of HALDevice.scsi_controller. >> + >> + Variant for a SCSI device connected to a PCI controller. >> + """ >> + devices = [ >> + # The PCI host controller. >> + { >> + 'id': 1, >> + 'udi': self.UDI_SATA_CONTROLLER, >> + 'properties': { >> + 'info.bus': ('pci', 'str'), >> + 'pci.device_class': (PCI_CLASS_STORAGE, 'int'), >> + 'pci.device_subclass': (PCI_SUBCLASS_STORAGE_SATA, >> + 'int'), >> + }, >> + }, >> + # The (fake or real) SCSI host of the storage device. >> + { >> + 'id': 2, >> + 'udi': self.UDI_SATA_CONTROLLER_SCSI, >> + 'properties': { >> + 'info.parent': (self.UDI_SATA_CONTROLLER, 'str'), >> + }, >> + }, >> + # The (possibly fake) SCSI disk. >> + { >> + 'id': 3, >> + 'udi': self.UDI_SATA_DISK, >> + 'properties': { >> + 'info.bus': ('scsi', 'str'), >> + 'info.parent': (self.UDI_SATA_CONTROLLER_SCSI, 'str'), >> + }, >> + }, >> + ] >> + parsed_data = { >> + 'hardware': { >> + 'hal': { >> + 'devices': devices, >> + }, >> + }, >> + } >> + >> + parser = SubmissionParser() >> + parser.buildDeviceList(parsed_data) >> + >> + scsi_device = parser.hal_devices[self.UDI_SATA_DISK] >> + controller = parser.hal_devices[self.UDI_SATA_CONTROLLER] >> + self.assertEqual(controller, scsi_device.scsi_controller) >> + >> + def test_HALDevice_scsi_controller_non_scsi_device(self): >> + """test of HALDevice.scsi_controller. >> + >> + Variant for non-SCSI devices. >> + """ >> + devices = [ >> + { >> + 'id': 1, >> + 'udi': self.UDI_COMPUTER, >> + 'properties': {}, >> + }, >> + ] >> + parsed_data = { >> + 'hardware': { >> + 'hal': { >> + 'devices': devices, >> + }, >> + }, >> + } >> + >> + parser = SubmissionParser() >> + parser.buildDeviceList(parsed_data) >> + >> + device = parser.hal_devices[self.UDI_COMPUTER] >> + self.assertEqual(None, device.scsi_controller) >> + >> + def test_HALDevice_scsi_controller_no_grandparent(self): >> + """test of HALDevice.scsi_controller. >> + >> + Variant for a SCSI device without a grandparent device. >> + """ >> + devices = [ >> + # The (fake or real) SCSI host of the storage device. >> + { >> + 'id': 1, >> + 'udi': self.UDI_SATA_CONTROLLER_SCSI, >> + 'properties': {}, >> + }, >> + # The (possibly fake) SCSI disk. >> + { >> + 'id': 2, >> + 'udi': self.UDI_SATA_DISK, >> + 'properties': { >> + 'info.bus': ('scsi', 'str'), >> + 'info.parent': (self.UDI_SATA_CONTROLLER_SCSI, 'str'), >> + }, >> + }, >> + ] >> + parsed_data = { >> + 'hardware': { >> + 'hal': { >> + 'devices': devices, >> + }, >> + }, >> + } >> + >> + parser = SubmissionParser(self.log) >> + parser.submission_key = 'SCSI device without grandparent device' >> + parser.buildDeviceList(parsed_data) >> + >> + scsi_device = parser.hal_devices[self.UDI_SATA_DISK] >> + self.assertEqual(None, scsi_device.scsi_controller) >> + self.assertWarningMessage( >> + parser.submission_key, >> + "Found SCSI device without a grandparent: %s." >> + % self.UDI_SATA_DISK) >> + >> + def test_HALDevice_scsi_controller_no_parent(self): >> + """test of HALDevice.scsi_controller. >> + >> + Variant for a SCSI device without a parent device. >> + """ >> + devices = [ >> + # The (possibly fake) SCSI disk. >> + { >> + 'id': 1, >> + 'udi': self.UDI_SATA_DISK, >> + 'properties': { >> + 'info.bus': ('scsi', 'str'), >> + }, >> + }, >> + ] >> + parsed_data = { >> + 'hardware': { >> + 'hal': { >> + 'devices': devices, >> + }, >> + }, >> + } >> + >> + parser = SubmissionParser(self.log) >> + parser.submission_key = 'SCSI device without parent device' >> + parser.buildDeviceList(parsed_data) >> + >> + scsi_device = parser.hal_devices[self.UDI_SATA_DISK] >> + self.assertEqual(None, scsi_device.scsi_controller) >> + self.assertWarningMessage( >> + parser.submission_key, >> + "Found SCSI device without a parent: %s." % self.UDI_SATA_DISK) >> + >> def testHALDeviceGetRealBus(self): >> """Test of HALDevice.real_bus, generic case. >> >> @@ -2585,11 +2793,9 @@ >> 'property not treated as a real device.') >> >> >> -class TestUdevDevice(TestCase): >> +class TestUdevDevice(TestCaseHWDB): >> """Tests of class UdevDevice.""" >> >> - layer = BaseLayer >> - >> root_device = { >> 'P': '/devices/LNXSYSTM:00', >> 'E': { >> @@ -2613,6 +2819,7 @@ >> 'PCI_SUBSYS_ID': '10CF:1387', >> 'PCI_SLOT_NAME': '0000:00:1f.2', >> 'SUBSYSTEM': 'pci', >> + 'DRIVER': 'ahci', >> } >> } >> >> @@ -2623,22 +2830,194 @@ >> 'DEVTYPE': 'usb_device', >> 'PRODUCT': '46d/a01/1013', >> 'TYPE': '0/0/0', >> - }, >> - } >> - >> - scsi_device_data = { >> - 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0', >> - 'E': { >> - 'SUBSYSTEM': 'scsi', >> - 'DEVTYPE': 'scsi_device', >> - }, >> - } >> - >> - scsi_device_sysfs_data = { >> - 'vendor': 'MATSHITA', >> - 'model': 'DVD-RAM UJ-841S', >> - 'type': '5', >> - } >> + 'DRIVER': 'usb', >> + }, >> + } >> + >> + pci_scsi_controller_data = { >> + 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0', >> + 'E': { >> + 'DRIVER': 'aic7xxx', >> + 'PCI_CLASS': '10000', >> + 'PCI_ID': '9004:6075', >> + 'PCI_SUBSYS_ID': '9004:7560', >> + 'SUBSYSTEM': 'pci', >> + }, >> + } >> + >> + pci_scsi_controller_scsi_side_1 = { >> + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' >> + 'host6'), >> + 'E': { >> + 'DEVTYPE': 'scsi_host', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + pci_scsi_controller_scsi_side_2 = { >> + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' >> + 'host6/scsi_host/host6'), >> + 'E': { >> + 'SUBSYSTEM': 'scsi_host', >> + }, >> + } >> + >> + scsi_scanner_target_data = { >> + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' >> + 'host6/target6:0:1'), >> + 'E': { >> + 'DEVTYPE': 'scsi_target', >> + 'SUBSYSTEM': 'scsi' >> + }, >> + } >> + >> + scsi_scanner_device_data = { >> + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' >> + 'host6/target6:0:1/6:0:1:0'), >> + 'E': { >> + 'DEVTYPE': 'scsi_device', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + scsi_scanner_device_sysfs_data = { >> + 'vendor': 'FUJITSU', >> + 'model': 'fi-5120Cdj', >> + 'type': '6', >> + } >> + >> + scsi_device_hierarchy_data = [ >> + {'udev_data': pci_scsi_controller_data}, >> + {'udev_data': pci_scsi_controller_scsi_side_1}, >> + {'udev_data': pci_scsi_controller_scsi_side_2}, >> + {'udev_data': scsi_scanner_target_data}, >> + { >> + 'udev_data': scsi_scanner_device_data, >> + 'sysfs_data': scsi_scanner_device_sysfs_data, >> + }, >> + ] >> + >> + pci_ide_controller = { >> + 'P': '/devices/pci0000:00/0000:00:1f.1', >> + 'E': { >> + 'DRIVER': 'ata_piix', >> + 'PCI_CLASS': '1018A', >> + 'PCI_ID': '8086:27DF', >> + 'PCI_SUBSYS_ID': '10CF:1385', >> + 'SUBSYSTEM': 'pci', >> + }, >> + } >> + >> + pci_ide_controller_scsi_side_1 = { >> + 'P': '/devices/pci0000:00/0000:00:1f.1/host4', >> + 'E': { >> + 'DEVTYPE': 'scsi_host', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + pci_ide_controller_scsi_side_2 = { >> + 'P': '/devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4', >> + 'E': { >> + 'SUBSYSTEM': 'scsi_host', >> + }, >> + } >> + >> + ide_device_target_data = { >> + 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0', >> + 'E': { >> + 'DEVTYPE': 'scsi_target', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + ide_cdrom_device_data = { >> + 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0', >> + 'E': { >> + 'SUBSYSTEM': 'scsi', >> + 'DEVTYPE': 'scsi_device', >> + 'DRIVER': 'sr', >> + }, >> + } >> + >> + ide_cdrom_device_sysfs_data = { >> + 'vendor': 'MATSHITA', >> + 'model': 'DVD-RAM UJ-841S', >> + 'type': '5', >> + } >> + >> + ide_device_hierarchy_data = [ >> + {'udev_data': pci_ide_controller}, >> + {'udev_data': pci_ide_controller_scsi_side_1}, >> + {'udev_data': pci_ide_controller_scsi_side_2}, >> + {'udev_data': ide_device_target_data}, >> + { >> + 'udev_data': ide_cdrom_device_data, >> + 'sysfs_data': ide_cdrom_device_sysfs_data, >> + }, >> + ] >> + >> + usb_storage_usb_interface = { >> + 'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0', >> + 'E': { >> + 'DRIVER': 'usb-storage', >> + 'PRODUCT': '1307/163/100', >> + 'TYPE': '0/0/0', >> + 'INTERFACE': '8/6/80', >> + 'SUBSYSTEM': 'usb', >> + }, >> + } >> + usb_storage_scsi_host_1 = { >> + 'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7', >> + 'E': { >> + 'DEVTYPE': 'scsi_host', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + usb_storage_scsi_host_2 = { >> + 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' >> + 'scsi_host/host7'), >> + 'E': { >> + 'SUBSYSTEM': 'scsi_host', >> + }, >> + } >> + >> + usb_storage_scsi_target = { >> + 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' >> + 'target7:0:0'), >> + 'E': { >> + 'DEVTYPE': 'scsi_target', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + usb_storage_scsi_device = { >> + 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' >> + 'target7:0:0/7:0:0:0'), >> + 'E': { >> + 'DEVTYPE': 'scsi_device', >> + 'DRIVER': 'sd', >> + 'SUBSYSTEM': 'scsi', >> + }, >> + } >> + >> + usb_storage_scsi_device_sysfs = { >> + 'vendor': 'Ut163', >> + 'model': 'USB2FlashStorage', >> + 'type': '0', >> + } >> + >> + usb_storage_hierarchy_data = [ >> + {'udev_data': usb_storage_usb_interface}, >> + {'udev_data': usb_storage_scsi_host_1}, >> + {'udev_data': usb_storage_scsi_host_2}, >> + {'udev_data': usb_storage_scsi_target}, >> + { >> + 'udev_data': usb_storage_scsi_device, >> + 'sysfs_data': usb_storage_scsi_device_sysfs, >> + }, >> + ] > > I'll have to trust you on the values of all that test data, but shoudn't > it all be created as part of setUp() - so that if an individual test > modifies the data in some way, it won't affect other tests? As it is I think > it could lead to some hard-to-debug failures in the future? The added benefit > is that you may be able to get away with less test data, as individual > tests can update the values of the test data as they need. > > Oh, I see below that the way you've done it makes your tests very clear > (just creating UdevDevices based on different combinations of the above). > OK, so if it wouldn't help at all, forget my point in the previous > paragraph (although it would be good to ensure the data is somehow immutable > if it stays there as class attributes?) Well, a change of the default test data might nevertheless happen in a future test. So I moved the definitions into setUp(). > >> >> no_subsystem_device_data = { >> 'P': '/devices/pnp0/00:00', >> @@ -2790,7 +3169,8 @@ >> def test_is_scsi_device(self): >> """Test of UdevDevice.is_scsi_device.""" >> device = UdevDevice( >> - None, self.scsi_device_data, self.scsi_device_sysfs_data) >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> self.assertTrue(device.is_scsi_device) >> >> device = UdevDevice(None, self.root_device) >> @@ -2799,16 +3179,18 @@ >> def test_scsi_vendor(self): >> """Test of UdevDevice.scsi_vendor.""" >> device = UdevDevice( >> - None, self.scsi_device_data, self.scsi_device_sysfs_data, None) >> - self.assertEqual('MATSHITA', device.scsi_vendor) >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> + self.assertEqual('FUJITSU', device.scsi_vendor) >> device = UdevDevice(None, self.root_device) >> self.assertEqual(None, device.scsi_vendor) >> >> def test_scsi_model(self): >> """Test of UdevDevice.scsi_model.""" >> device = UdevDevice( >> - None, self.scsi_device_data, self.scsi_device_sysfs_data) >> - self.assertEqual('DVD-RAM UJ-841S', device.scsi_model) >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> + self.assertEqual('fi-5120Cdj', device.scsi_model) >> >> device = UdevDevice(None, self.root_device) >> self.assertEqual(None, device.scsi_model) >> @@ -2847,10 +3229,10 @@ >> self.assertEqual('Unknown', device.getVendorOrProduct('product')) >> >> device = UdevDevice( >> - None, self.scsi_device_data, self.scsi_device_sysfs_data) >> - self.assertEqual('MATSHITA', device.getVendorOrProduct('vendor')) >> - self.assertEqual( >> - 'DVD-RAM UJ-841S', device.getVendorOrProduct('product')) >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> + self.assertEqual('FUJITSU', device.getVendorOrProduct('vendor')) >> + self.assertEqual('fi-5120Cdj', device.getVendorOrProduct('product')) >> >> device = UdevDevice(None, self.no_subsystem_device_data) >> self.assertEqual(None, device.getVendorOrProduct('vendor')) >> @@ -2888,10 +3270,10 @@ >> self.assertEqual(0xa01, device.getVendorOrProductID('product')) >> >> device = UdevDevice( >> - None, self.scsi_device_data, self.scsi_device_sysfs_data) >> - self.assertEqual('MATSHITA', device.getVendorOrProductID('vendor')) >> - self.assertEqual( >> - 'DVD-RAM UJ-841S', device.getVendorOrProductID('product')) >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> + self.assertEqual('FUJITSU', device.getVendorOrProductID('vendor')) >> + self.assertEqual('fi-5120Cdj', device.getVendorOrProductID('product')) >> >> device = UdevDevice( >> None, self.no_subsystem_device_data) >> @@ -2910,6 +3292,132 @@ >> None, self.root_device, None, self.root_device_dmi_data) >> self.assertEqual('LIFEBOOK E8210', device.product_id) >> >> + def test_vendor_id_for_db(self): >> + """Test of UdevDevice.vendor_id_for_db.""" >> + device = UdevDevice( >> + None, self.root_device, None, self.root_device_dmi_data) >> + self.assertEqual('FUJITSU SIEMENS', device.vendor_id_for_db) >> + >> + device = UdevDevice(None, self.pci_device_data) >> + self.assertEqual('0x8086', device.vendor_id_for_db) >> + >> + device = UdevDevice(None, self.usb_device_data) >> + self.assertEqual('0x046d', device.vendor_id_for_db) >> + >> + device = UdevDevice( >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> + self.assertEqual('FUJITSU ', device.vendor_id_for_db) >> + >> + def test_product_id_for_db(self): >> + """Test of UdevDevice.product_id_for_db.""" >> + device = UdevDevice( >> + None, self.root_device, None, self.root_device_dmi_data) >> + self.assertEqual('LIFEBOOK E8210', device.product_id_for_db) >> + >> + device = UdevDevice(None, self.pci_device_data) >> + self.assertEqual('0x27c5', device.product_id_for_db) >> + >> + device = UdevDevice(None, self.usb_device_data) >> + self.assertEqual('0x0a01', device.product_id_for_db) >> + >> + device = UdevDevice( >> + None, self.scsi_scanner_device_data, >> + self.scsi_scanner_device_sysfs_data) >> + self.assertEqual('fi-5120Cdj ', device.product_id_for_db) >> + >> + def test_driver_name(self): >> + """Test of UdevDevice.driver_name.""" >> + device = UdevDevice(None, self.pci_device_data) >> + self.assertEqual('ahci', device.driver_name) >> + >> + device = UdevDevice( >> + None, self.root_device, None, self.root_device_dmi_data) >> + self.assertEqual(None, device.driver_name) >> + >> + def buildUdevDeviceHierarchy(self, device_data, parser=None): >> + """Build a UdevDevice hierarchy from device_data. >> + >> + :param device_data: A sequence of arguments that are passed >> + to the UdevDevice constructor. Each element must be a >> + dictionary that can be used as a **kwargs argument. >> + >> + Element N if the sequence is the parent of element N+1. >> + is the ancestor of the fo >> + :param parser: A SubmissionParser instance to be passed to >> + the constructor of UdevDevice. >> + """ >> + parent = None >> + devices = [] >> + for kwargs in device_data: >> + device = UdevDevice(parser, **kwargs) >> + devices.append(device) >> + if parent is not None: >> + parent.addChild(device) >> + parent = device >> + return devices >> + >> + def test_scsi_controller(self): >> + """Test of UdevDevice.scsi_controller for a PCI controller.""" >> + devices = self.buildUdevDeviceHierarchy( >> + self.scsi_device_hierarchy_data) >> + controller = devices[0] >> + scsi_device = devices[-1] >> + self.assertEqual(controller, scsi_device.scsi_controller) >> + >> + def test_scsi_controller_insufficient_anchestors(self): >> + """Test of UdevDevice.scsi_controller for a PCI controller. >> + >> + If a SCSI device does not have a sufficient number of ancestors, >> + UdevDevice.scsi_controller returns None. >> + """ >> + parser = SubmissionParser(self.log) >> + parser.submission_key = 'UdevDevice.scsi_controller ancestor missing' >> + devices = self.buildUdevDeviceHierarchy( >> + self.scsi_device_hierarchy_data[1:], parser) >> + scsi_device = devices[-1] >> + self.assertEqual(None, scsi_device.scsi_controller) >> + self.assertWarningMessage( >> + parser.submission_key, >> + 'Found a SCSI device without a sufficient number of ancestors: ' >> + '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' >> + 'host6/target6:0:1/6:0:1:0') >> + >> + def test_scsi_controller_no_scsi_device(self): >> + """Test of UdevDevice.scsi_controller for a PCI controller. >> + >> + For non-SCSI devices, this property is None. >> + """ >> + device = UdevDevice(None, self.pci_device_data) >> + self.assertEqual(None, device.scsi_controller) >> + >> + def test_translateScsiBus_real_scsi_device(self): >> + """Test of UdevDevice.translateScsiBus() with a real SCSI device.""" >> + devices = self.buildUdevDeviceHierarchy( >> + self.scsi_device_hierarchy_data) >> + scsi_device = devices[-1] >> + self.assertEqual( >> + HWBus.SCSI, scsi_device.translateScsiBus()) >> + >> + def test_translateScsiBus_ide_device(self): >> + """Test of UdevDevice.translateScsiBus() with an IDE device.""" >> + devices = self.buildUdevDeviceHierarchy( >> + self.ide_device_hierarchy_data) >> + ide_device = devices[-1] >> + self.assertEqual(HWBus.IDE, ide_device.translateScsiBus()) >> + >> + def test_translateScsiBus_usb_device(self): >> + """Test of UdevDevice.translateScsiBus() with a USB device.""" >> + devices = self.buildUdevDeviceHierarchy( >> + self.usb_storage_hierarchy_data) >> + usb_scsi_device = devices[-1] >> + self.assertEqual(None, usb_scsi_device.translateScsiBus()) >> + >> + def test_translateScsiBus_non_scsi_device(self): >> + """Test of UdevDevice.translateScsiBus() for a non-SCSI device.""" >> + device = UdevDevice(None, self.root_device) >> + self.assertEqual(None, device.translateScsiBus()) >> + > > Wow - great work on the test front there! It's great to see a branch where > 3/4 of the diff is tests ensuring the code continues to work! One thing > that might help the readability for people like me who are not familiar > with HWDB stuff, would be to make it obvious why you are always choosing > the last item in the heirarchy (devices[-1]) for the scsi/usb device? A > comment would do, but then you'd need the comment in each test. Perhaps > defining: > > getDeviceFromHierarchy(devices) - where you could have a comment explaining > why you're simply returning the last device in the heirarchy? Not sure if this really helps. Sure, in most cases the last device of the list is of interest, but test_scsi_controller() for example accesses the first and the last device. OTOH I think that the the doc string of buildUdevDeviceHierarchy() explains what is returned, and if you look at a test and the tested method/property with this in mind, it should be obvious why a given device is selected for an assertion. Abel === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py' --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 08:53:47 +0000 +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-15 13:02:39 +0000 @@ -2646,18 +2646,18 @@ if self.raw_bus != 'scsi_device': return None - current_device = self - for ancestor_level in range(1, 5): - if current_device.parent is None: - # While SCSI devices from valid submissions should have a - # parent and a grandparent, we can't be sure for bogus or - # broken submissions. - self.parser._logWarning( - 'Found a SCSI device without a sufficient number of ' - 'ancestors: %s' % self.device_id) - return None - current_device = current_device.parent - return current_device + # While SCSI devices from valid submissions should have four + # ancestors, we can't be sure for bogus or broken submissions. + try: + controller = self.parent.parent.parent.parent + except AttributeError: + controller = None + if controller is None: + self.parser._logWarning( + 'Found a SCSI device without a sufficient number of ' + 'ancestors: %s' % self.device_id) + return None + return controller class ProcessingLoop(object): """An `ITunableLoop` for processing HWDB submissions.""" === 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 08:53:47 +0000 +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-15 13:14:18 +0000 @@ -2796,233 +2796,237 @@ class TestUdevDevice(TestCaseHWDB): """Tests of class UdevDevice.""" - root_device = { - 'P': '/devices/LNXSYSTM:00', - 'E': { - 'UDEV_LOG': '3', - 'DEVPATH': '/devices/LNXSYSTM:00', - 'MODALIAS': 'acpi:LNXSYSTM:', - 'SUBSYSTEM': 'acpi', - } - } - - root_device_dmi_data = { - '/sys/class/dmi/id/sys_vendor': 'FUJITSU SIEMENS', - '/sys/class/dmi/id/product_name': 'LIFEBOOK E8210', - } - - pci_device_data = { - 'P': '/devices/pci0000:00/0000:00:1f.2', - 'E': { - 'PCI_CLASS': '10602', - 'PCI_ID': '8086:27C5', - 'PCI_SUBSYS_ID': '10CF:1387', - 'PCI_SLOT_NAME': '0000:00:1f.2', - 'SUBSYSTEM': 'pci', - 'DRIVER': 'ahci', - } - } - - usb_device_data = { - 'P': '/devices/pci0000:00/0000:00:1d.1/usb3/3-2', - 'E': { - 'SUBSYSTEM': 'usb', - 'DEVTYPE': 'usb_device', - 'PRODUCT': '46d/a01/1013', - 'TYPE': '0/0/0', - 'DRIVER': 'usb', - }, - } - - pci_scsi_controller_data = { - 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0', - 'E': { - 'DRIVER': 'aic7xxx', - 'PCI_CLASS': '10000', - 'PCI_ID': '9004:6075', - 'PCI_SUBSYS_ID': '9004:7560', - 'SUBSYSTEM': 'pci', - }, - } - - pci_scsi_controller_scsi_side_1 = { - 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' - 'host6'), - 'E': { - 'DEVTYPE': 'scsi_host', - 'SUBSYSTEM': 'scsi', - }, - } - - pci_scsi_controller_scsi_side_2 = { - 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' - 'host6/scsi_host/host6'), - 'E': { - 'SUBSYSTEM': 'scsi_host', - }, - } - - scsi_scanner_target_data = { - 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' - 'host6/target6:0:1'), - 'E': { - 'DEVTYPE': 'scsi_target', - 'SUBSYSTEM': 'scsi' - }, - } - - scsi_scanner_device_data = { - 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' - 'host6/target6:0:1/6:0:1:0'), - 'E': { - 'DEVTYPE': 'scsi_device', - 'SUBSYSTEM': 'scsi', - }, - } - - scsi_scanner_device_sysfs_data = { - 'vendor': 'FUJITSU', - 'model': 'fi-5120Cdj', - 'type': '6', - } - - scsi_device_hierarchy_data = [ - {'udev_data': pci_scsi_controller_data}, - {'udev_data': pci_scsi_controller_scsi_side_1}, - {'udev_data': pci_scsi_controller_scsi_side_2}, - {'udev_data': scsi_scanner_target_data}, - { - 'udev_data': scsi_scanner_device_data, - 'sysfs_data': scsi_scanner_device_sysfs_data, - }, - ] - - pci_ide_controller = { - 'P': '/devices/pci0000:00/0000:00:1f.1', - 'E': { - 'DRIVER': 'ata_piix', - 'PCI_CLASS': '1018A', - 'PCI_ID': '8086:27DF', - 'PCI_SUBSYS_ID': '10CF:1385', - 'SUBSYSTEM': 'pci', - }, - } - - pci_ide_controller_scsi_side_1 = { - 'P': '/devices/pci0000:00/0000:00:1f.1/host4', - 'E': { - 'DEVTYPE': 'scsi_host', - 'SUBSYSTEM': 'scsi', - }, - } - - pci_ide_controller_scsi_side_2 = { - 'P': '/devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4', - 'E': { - 'SUBSYSTEM': 'scsi_host', - }, - } - - ide_device_target_data = { - 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0', - 'E': { - 'DEVTYPE': 'scsi_target', - 'SUBSYSTEM': 'scsi', - }, - } - - ide_cdrom_device_data = { - 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0', - 'E': { - 'SUBSYSTEM': 'scsi', - 'DEVTYPE': 'scsi_device', - 'DRIVER': 'sr', - }, - } - - ide_cdrom_device_sysfs_data = { - 'vendor': 'MATSHITA', - 'model': 'DVD-RAM UJ-841S', - 'type': '5', - } - - ide_device_hierarchy_data = [ - {'udev_data': pci_ide_controller}, - {'udev_data': pci_ide_controller_scsi_side_1}, - {'udev_data': pci_ide_controller_scsi_side_2}, - {'udev_data': ide_device_target_data}, - { - 'udev_data': ide_cdrom_device_data, - 'sysfs_data': ide_cdrom_device_sysfs_data, - }, - ] - - usb_storage_usb_interface = { - 'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0', - 'E': { - 'DRIVER': 'usb-storage', - 'PRODUCT': '1307/163/100', - 'TYPE': '0/0/0', - 'INTERFACE': '8/6/80', - 'SUBSYSTEM': 'usb', - }, - } - usb_storage_scsi_host_1 = { - 'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7', - 'E': { - 'DEVTYPE': 'scsi_host', - 'SUBSYSTEM': 'scsi', - }, - } - - usb_storage_scsi_host_2 = { - 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' - 'scsi_host/host7'), - 'E': { - 'SUBSYSTEM': 'scsi_host', - }, - } - - usb_storage_scsi_target = { - 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' - 'target7:0:0'), - 'E': { - 'DEVTYPE': 'scsi_target', - 'SUBSYSTEM': 'scsi', - }, - } - - usb_storage_scsi_device = { - 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' - 'target7:0:0/7:0:0:0'), - 'E': { - 'DEVTYPE': 'scsi_device', - 'DRIVER': 'sd', - 'SUBSYSTEM': 'scsi', - }, - } - - usb_storage_scsi_device_sysfs = { - 'vendor': 'Ut163', - 'model': 'USB2FlashStorage', - 'type': '0', - } - - usb_storage_hierarchy_data = [ - {'udev_data': usb_storage_usb_interface}, - {'udev_data': usb_storage_scsi_host_1}, - {'udev_data': usb_storage_scsi_host_2}, - {'udev_data': usb_storage_scsi_target}, - { - 'udev_data': usb_storage_scsi_device, - 'sysfs_data': usb_storage_scsi_device_sysfs, - }, - ] - - no_subsystem_device_data = { - 'P': '/devices/pnp0/00:00', - 'E': {} - } + def setUp(self): + """Setup the test environment.""" + super(TestUdevDevice, self).setUp() + self.root_device = { + 'P': '/devices/LNXSYSTM:00', + 'E': { + 'UDEV_LOG': '3', + 'DEVPATH': '/devices/LNXSYSTM:00', + 'MODALIAS': 'acpi:LNXSYSTM:', + 'SUBSYSTEM': 'acpi', + } + } + + self.root_device_dmi_data = { + '/sys/class/dmi/id/sys_vendor': 'FUJITSU SIEMENS', + '/sys/class/dmi/id/product_name': 'LIFEBOOK E8210', + } + + self.pci_device_data = { + 'P': '/devices/pci0000:00/0000:00:1f.2', + 'E': { + 'PCI_CLASS': '10602', + 'PCI_ID': '8086:27C5', + 'PCI_SUBSYS_ID': '10CF:1387', + 'PCI_SLOT_NAME': '0000:00:1f.2', + 'SUBSYSTEM': 'pci', + 'DRIVER': 'ahci', + } + } + + self.usb_device_data = { + 'P': '/devices/pci0000:00/0000:00:1d.1/usb3/3-2', + 'E': { + 'SUBSYSTEM': 'usb', + 'DEVTYPE': 'usb_device', + 'PRODUCT': '46d/a01/1013', + 'TYPE': '0/0/0', + 'DRIVER': 'usb', + }, + } + + self.pci_scsi_controller_data = { + 'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0', + 'E': { + 'DRIVER': 'aic7xxx', + 'PCI_CLASS': '10000', + 'PCI_ID': '9004:6075', + 'PCI_SUBSYS_ID': '9004:7560', + 'SUBSYSTEM': 'pci', + }, + } + + self.pci_scsi_controller_scsi_side_1 = { + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' + 'host6'), + 'E': { + 'DEVTYPE': 'scsi_host', + 'SUBSYSTEM': 'scsi', + }, + } + + self.pci_scsi_controller_scsi_side_2 = { + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' + 'host6/scsi_host/host6'), + 'E': { + 'SUBSYSTEM': 'scsi_host', + }, + } + + self.scsi_scanner_target_data = { + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' + 'host6/target6:0:1'), + 'E': { + 'DEVTYPE': 'scsi_target', + 'SUBSYSTEM': 'scsi' + }, + } + + self.scsi_scanner_device_data = { + 'P': ('/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0/' + 'host6/target6:0:1/6:0:1:0'), + 'E': { + 'DEVTYPE': 'scsi_device', + 'SUBSYSTEM': 'scsi', + }, + } + + self.scsi_scanner_device_sysfs_data = { + 'vendor': 'FUJITSU', + 'model': 'fi-5120Cdj', + 'type': '6', + } + + self.scsi_device_hierarchy_data = [ + {'udev_data': self.pci_scsi_controller_data}, + {'udev_data': self.pci_scsi_controller_scsi_side_1}, + {'udev_data': self.pci_scsi_controller_scsi_side_2}, + {'udev_data': self.scsi_scanner_target_data}, + { + 'udev_data': self.scsi_scanner_device_data, + 'sysfs_data': self.scsi_scanner_device_sysfs_data, + }, + ] + + self.pci_ide_controller = { + 'P': '/devices/pci0000:00/0000:00:1f.1', + 'E': { + 'DRIVER': 'ata_piix', + 'PCI_CLASS': '1018A', + 'PCI_ID': '8086:27DF', + 'PCI_SUBSYS_ID': '10CF:1385', + 'SUBSYSTEM': 'pci', + }, + } + + self.pci_ide_controller_scsi_side_1 = { + 'P': '/devices/pci0000:00/0000:00:1f.1/host4', + 'E': { + 'DEVTYPE': 'scsi_host', + 'SUBSYSTEM': 'scsi', + }, + } + + self.pci_ide_controller_scsi_side_2 = { + 'P': '/devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4', + 'E': { + 'SUBSYSTEM': 'scsi_host', + }, + } + + self.ide_device_target_data = { + 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0', + 'E': { + 'DEVTYPE': 'scsi_target', + 'SUBSYSTEM': 'scsi', + }, + } + + self.ide_cdrom_device_data = { + 'P': '/devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0', + 'E': { + 'SUBSYSTEM': 'scsi', + 'DEVTYPE': 'scsi_device', + 'DRIVER': 'sr', + }, + } + + self.ide_cdrom_device_sysfs_data = { + 'vendor': 'MATSHITA', + 'model': 'DVD-RAM UJ-841S', + 'type': '5', + } + + self.ide_device_hierarchy_data = [ + {'udev_data': self.pci_ide_controller}, + {'udev_data': self.pci_ide_controller_scsi_side_1}, + {'udev_data': self.pci_ide_controller_scsi_side_2}, + {'udev_data': self.ide_device_target_data}, + { + 'udev_data': self.ide_cdrom_device_data, + 'sysfs_data': self.ide_cdrom_device_sysfs_data, + }, + ] + + self.usb_storage_usb_interface = { + 'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0', + 'E': { + 'DRIVER': 'usb-storage', + 'PRODUCT': '1307/163/100', + 'TYPE': '0/0/0', + 'INTERFACE': '8/6/80', + 'SUBSYSTEM': 'usb', + }, + } + + self.usb_storage_scsi_host_1 = { + 'P': '/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7', + 'E': { + 'DEVTYPE': 'scsi_host', + 'SUBSYSTEM': 'scsi', + }, + } + + self.usb_storage_scsi_host_2 = { + 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' + 'scsi_host/host7'), + 'E': { + 'SUBSYSTEM': 'scsi_host', + }, + } + + self.usb_storage_scsi_target = { + 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' + 'target7:0:0'), + 'E': { + 'DEVTYPE': 'scsi_target', + 'SUBSYSTEM': 'scsi', + }, + } + + self.usb_storage_scsi_device = { + 'P': ('/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0/host7/' + 'target7:0:0/7:0:0:0'), + 'E': { + 'DEVTYPE': 'scsi_device', + 'DRIVER': 'sd', + 'SUBSYSTEM': 'scsi', + }, + } + + self.usb_storage_scsi_device_sysfs = { + 'vendor': 'Ut163', + 'model': 'USB2FlashStorage', + 'type': '0', + } + + self.usb_storage_hierarchy_data = [ + {'udev_data': self.usb_storage_usb_interface}, + {'udev_data': self.usb_storage_scsi_host_1}, + {'udev_data': self.usb_storage_scsi_host_2}, + {'udev_data': self.usb_storage_scsi_target}, + { + 'udev_data': self.usb_storage_scsi_device, + 'sysfs_data': self.usb_storage_scsi_device_sysfs, + }, + ] + + self.no_subsystem_device_data = { + 'P': '/devices/pnp0/00:00', + 'E': {} + } def test_device_id(self): """Test of UdevDevice.device_id.""" @@ -3342,8 +3346,7 @@ to the UdevDevice constructor. Each element must be a dictionary that can be used as a **kwargs argument. - Element N if the sequence is the parent of element N+1. - is the ancestor of the fo + Element N of the sequence is the parent of element N+1. :param parser: A SubmissionParser instance to be passed to the constructor of UdevDevice. """