Launchpad itself

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

hwdb-class-udev-device-6
Merge into devel

Proposed by Abel Deuring on 2009-10-15

Status:	Merged
Merged at revision:	not available
Proposed branch:	lp:~adeuring/launchpad/hwdb-class-udev-device-6
Merge into:	lp:launchpad
Diff against target:	849 lines 2 files modified lib/canonical/launchpad/scripts/hwdbsubmissions.py (+61/-20) lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py (+582/-70)
To merge this branch:	bzr merge lp:~adeuring/launchpad/hwdb-class-udev-device-6
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Nelson (community)	code	2009-10-15	Approve on 2009-10-15
Review via email: mp+13402@code.launchpad.net

Revision history for this message

Abel Deuring (adeuring) wrote on 2009-10-15:

This branch adds a property scsi_controller to class UdevDevice in the script hwdbsubmissons.py.

Aside from tests of this property, I also added tests for UdevDevice.translateScsiBus(). While this method is defined in class BaseDevice, from which UdevDevice is derived, is accesses the property scsi_controller, so some tests seemed reasonable. (class UdevDevice represents a device mentioned in a submission made by Karmic's HWDB client, while submissions from older Ubuntu versions contain data from HAL; these devices are represnted by class HALDevice.)

Handling of SCSI devices in HWDB submissions is a bit convoluted: Many non-SCSI storage devices, like IDE/SATA disks and CD drives and USB storage devices, support the SCSI command set, and the Linux kernel treats them like real SCSI devices. The data stored about a device includes the bus used to connect the device to a computer, so we want to know the real, physical bus of a device, instead of treating nearly all storage devices as SCSI devices.

We can detect the real bus a of (real or fake) SCSI device by inspecting the type of the (real or fake) SCSI controller. This is done by BaseDevice.translateScsiBus(). The core logic of this method is the same for submissions containing HAL and udev data, so it is located in class BaseDevice. The difference between HAL and udev data is that the controller of a SCSI device in HAL is the grandparent of the SCSI device, while it is the grand-grand-grand-parent in udev, hence the property scsi_controller is differently implemented in HALDevice and UdevDevice.

In order to setup a test of UdevDevice.scsi_controller or UdevDevice.translateScsiBus, we need to create five UdevDevices, so I added a method buildUdevDeviceHierarchy() to class TestUdevDevice.

I also changed the base class of TestUdevDevice from TestCase to testCaseHWDB: The latter defines some infrastructure to assert that expected warnings or error messages really appear.

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)

This complaint is not related to my changes.

This branch adds a property scsi_controller to class UdevDevice in the script hwdbsubmissons.py.

In order to setup a test of UdevDevice.scsi_controller or UdevDevice.translateScsiBus, we need to create five UdevDevices, so I added a method buildUdevDeviceHierarchy() to class TestUdevDevice.

I also changed the base class of TestUdevDevice from TestCase to testCaseHWDB: The latter defines some infrastructure to assert that expected warnings or error messages really appear.

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)

This complaint is not related to my changes.

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2009-10-15:

Download full text (27.5 KiB)

> 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.

>
> 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,
            '...

> 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!

Up to you.

>  
>  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! :)

> +            # 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?)

>  
>      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?

Great work!

>  
>  class TestHWDBSubmissionTablePopulation(TestCaseHWDB):
>      """Tests of the HWDB popoluation with submitted data."""

-- 
Michael

review: Approve (code)

Revision history for this message

Abel Deuring (adeuring) wrote on 2009-10-15:

Download full text (45.5 KiB)

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.

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.
         """

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Abel Deuring

Barki Mustapha

Celso Providelo

Christian Reis

Christy Awad

Colin Watson

Harpianto,ANDI

James Troup

John A Meinel

Kevin bush

Launchpad code reviewers

Launchpad code reviewers from Canonical

Matthew Tanner

Maximiliano Bertacchini

Oguz Ersoz

Simon Brakhane

Ubuntu-BR DevOps

William Grant

alhawiti

api.ng

pedro cavazos

todaioan

wenjingwen

to status/vote changes:

Tzaddi

Tzaddi Belding

Launchpad itself

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

Commit message

Description of the change

Preview Diff

Subscribers

 === 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 13:25:24 +0000
@@ -87,12 +87,14 @@
      'pci': '0x%04x',
      'usb_device': '0x%04x',
      'scsi': '%-8s',
++    'scsi_device': '%-8s',
+     }
  DB_FORMAT_FOR_PRODUCT_ID = {
      'pci': '0x%04x',
      'usb_device': '0x%04x',
      'scsi': '%-16s',
++    'scsi_device': '%-16s',
+     }
  UDEV_USB_DEVICE_PROPERTIES = set(('DEVTYPE', 'PRODUCT', 'TYPE'))
@@ -1609,6 +1611,11 @@
          """The name of the driver contolling this device. May be None."""
          raise NotImplementedError
++    @property
++    def scsi_controller(self):
++        """Return the SCSI host controller for this device."""
++        raise NotImplementedError
++
      def translateScsiBus(self):
          """Return the real bus of a device where raw_bus=='scsi'.
@@ -1617,37 +1624,27 @@
          for more details. This method determines the real bus
          of a device accessed via the kernel's SCSI subsystem.
          """
--        # While SCSI devices from valid submissions should have a
--        # parent and a grandparent, we can't be sure for bogus or
--        # broken submissions.
--        parent = self.parent
--        if parent is None:
--            self.parser._logWarning(
--                'Found SCSI device without a parent: %s.' % self.device_id)
--            return None
--        grandparent = parent.parent
--        if grandparent is None:
--            self.parser._logWarning(
--                'Found SCSI device without a grandparent: %s.'
--                % self.device_id)
++        scsi_controller = self.scsi_controller
++        if scsi_controller is None:
              return None
--        grandparent_bus = grandparent.raw_bus
--        if grandparent_bus == 'pci':
--            if (grandparent.pci_class != PCI_CLASS_STORAGE):
++        scsi_controller_bus = scsi_controller.raw_bus
++        if scsi_controller_bus == 'pci':
++            if (scsi_controller.pci_class != PCI_CLASS_STORAGE):
                  # This is not a storage class PCI device? This
                  # indicates a bug somewhere in HAL or in the hwdb
                  # client, or a fake submission.
--                device_class = grandparent.pci_class
++                device_class = scsi_controller.pci_class
                  self.parser._logWarning(
                      'A (possibly fake) SCSI device %s is connected to '
                      'PCI device %s that has the PCI device class %s; '
                      'expected class 1 (storage).'
--                    % (self.device_id, grandparent.device_id, device_class))
++                    % (self.device_id, scsi_controller.device_id,
++                       device_class))
                  return None
--            pci_subclass = grandparent.pci_subclass
++            pci_subclass = scsi_controller.pci_subclass
              return self.pci_storage_subclass_hwbus.get(pci_subclass)
--        elif grandparent_bus == 'usb':
++        elif scsi_controller_bus == 'usb':
              # USB storage devices have the following HAL device hierarchy:
              # - HAL node for the USB device. info.bus == 'usb_device',
              #   device class == 0, device subclass == 0
@@ -2357,6 +2354,27 @@
          """See `BaseDevice`."""
          return self.getVendorOrProductID('product')
++    @property
++    def scsi_controller(self):
++        """See `BaseDevice`."""
++        # While SCSI devices from valid submissions should have a
++        # parent and a grandparent, we can't be sure for bogus or
++        # broken submissions.
++        if self.raw_bus != 'scsi':
++            return None
++        parent = self.parent
++        if parent is None:
++            self.parser._logWarning(
++                'Found SCSI device without a parent: %s.' % self.device_id)
++            return None
++        grandparent = parent.parent
++        if grandparent is None:
++            self.parser._logWarning(
++                'Found SCSI device without a grandparent: %s.'
++                % self.device_id)
++            return None
++        return grandparent
++
  class UdevDevice(BaseDevice):
      """The representation of a udev device node."""
@@ -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
++
++        # 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-13 21:02:35 +0000
 +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py	2009-10-15 13:25:24 +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'),
++                    },
++                },
++            # 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,65 +2793,241 @@
              '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': {
--            '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',
--            }
--        }
--
--    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',
--            },
--        }
--
--    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',
--        }
--
--    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."""
@@ -2790,7 +3174,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 +3184,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 +3234,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 +3275,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 +3297,131 @@
              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 of the sequence is the parent of element N+1.
++        :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())
++
  class TestHWDBSubmissionTablePopulation(TestCaseHWDB):
      """Tests of the HWDB popoluation with submitted data."""