Great! The addition of the is_root_device property makes the code much
more readable.

As usual, just one or two comments below, in particular, one change for which I couldn't see the cause...

Thanks!

> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py	2009-10-15 13:22:14 +0000
> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py	2009-10-15 13:59:33 +0000
> @@ -1644,7 +1644,7 @@
>                  return None
>              pci_subclass = scsi_controller.pci_subclass
>              return self.pci_storage_subclass_hwbus.get(pci_subclass)
> -        elif scsi_controller_bus == 'usb':
> +        elif scsi_controller_bus in ('usb', 'usb_interface'):
>              # 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
> @@ -1703,6 +1703,12 @@
>              return HWBus.PCI
>  
>      @property
> +    def is_root_device(self):
> +        """Return True is this is the root node of all devicese, else False.
> +        """
> +        raise NotImplementedError
> +
> +    @property
>      def raw_bus(self):
>          """Return the device bus as specified by HAL or udev."""
>          raise NotImplementedError
> @@ -1719,11 +1725,11 @@
>          if result is not None:
>              return result
>  
> -        if device_bus == 'scsi':
> +        if device_bus in ('scsi', 'scsi_device'):
>              return self.translateScsiBus()
>          elif device_bus == 'pci':
>              return self.translatePciBus()
> -        elif self.udi == ROOT_UDI:
> +        elif self.is_root_device:
>              # The computer itself. In Hardy, HAL provides no info.bus
>              # for the machine itself; older versions set info.bus to
>              # 'unknown', hence it is better to use the machine's
> @@ -1731,7 +1737,7 @@
>              return HWBus.SYSTEM
>          else:
>              self.parser._logWarning(
> -                'Unknown bus %r for device %s' % (device_bus, self.udi))
> +                'Unknown bus %r for device %s' % (device_bus, self.device_id))
>              return None
>  
>      @property
> @@ -2275,6 +2281,11 @@
>              return result
>          return self.getProperty('info.subsystem')
>  
> +    @property
> +    def is_root_device(self):
> +        """See `BaseDevice`."""
> +        return self.udi == ROOT_UDI
> +
>      def getVendorOrProduct(self, type_):
>          """Return the vendor or product of this device.
>  
> @@ -2547,7 +2558,7 @@
>          # DEVTYPE. DEVTYPE is preferable.
>          # The root device has the subsystem/bus value "acpi", which
>          # is a bit nonsensical.
> -        if self.device_id == UDEV_ROOT_PATH:
> +        if self.is_root_device:
>              return None
>          properties = self.udev['E']
>          devtype = properties.get('DEVTYPE')
> @@ -2555,6 +2566,11 @@
>              return devtype
>          return properties.get('SUBSYSTEM')
>  
> +    @property
> +    def is_root_device(self):
> +        """See `BaseDevice`."""
> +        return self.udev['P'] == UDEV_ROOT_PATH
> +
>      def getVendorOrProduct(self, type_):
>          """Return the vendor or product of this device.
>  
> @@ -2565,7 +2581,7 @@
>              'Unexpected value of type_: %r' % type_)
>  
>          bus = self.raw_bus
> -        if self.device_id == UDEV_ROOT_PATH:
> +        if self.is_root_device:
>              # udev does not known about any product information for
>              # the root device. We use DMI data instead.
>              return self.root_device_ids[type_]
> @@ -2605,7 +2621,7 @@
>              'Unexpected value of type_: %r' % type_)
>  
>          bus = self.raw_bus
> -        if self.device_id == UDEV_ROOT_PATH:
> +        if self.is_root_device:
>              # udev does not known about any product information for
>              # the root device. We use DMI data instead.
>              if type_ == 'vendor':
> 
> === 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 13:22:14 +0000
> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py	2009-10-15 13:59:33 +0000
> @@ -1173,6 +1173,51 @@
>              parser.submission_key,
>              "Unknown bus 'nonsense' for device " + self.UDI_PCCARD_DEVICE)
>  
> +    def testHALDevice_is_root_device_for_root_device(self):

A few tests above use the style:

    def test_HALDevice_scsi_controller_no_parent(self):

(I think from a previous review?). This seems more consistant with

https://dev.launchpad.net/TestsStyleGuide#Python Test Cases

> +        """Test of HALDevice.is_root_device for the root device."""
> +        devices = [
> +            {
> +                'id': 1,
> +                'udi': self.UDI_COMPUTER,
> +                'properties': {},
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser()
> +        parser.submission_key = 'Test of HALDevice.is_root_device'
> +        parser.buildDeviceList(parsed_data)
> +        self.assertTrue(parser.hal_devices[self.UDI_COMPUTER].is_root_device)
> +
> +    def testHALDevice_is_root_device_for_non_root_device(self):
> +        """Test of HALDevice.is_root_device for a non-root device."""
> +        devices = [
> +            {
> +                'id': 1,
> +                'udi': self.UDI_PCCARD_DEVICE,
> +                'properties': {},
> +                },
> +            ]
> +        parsed_data = {
> +            'hardware': {
> +                'hal': {
> +                    'devices': devices,
> +                    },
> +                },
> +            }
> +
> +        parser = SubmissionParser()
> +        parser.submission_key = 'Test of HALDevice.is_root_device'
> +        parser.buildDeviceList(parsed_data)
> +        self.assertFalse(
> +            parser.hal_devices[self.UDI_PCCARD_DEVICE].is_root_device)
> +
>      def renameInfoBusToInfoSubsystem(self, devices):
>          """Rename the property info.bus in a device list to info.subsystem.
>  
> @@ -2837,7 +2882,19 @@
>                  },
>              }
>  
> -        self.pci_scsi_controller_data = {
> +        self.pci_pccard_bridge = {
> +            'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0',
> +            'E': {
> +                'DRIVER': 'yenta_cardbus',
> +                'PCI_CLASS': '60700',
> +                'PCI_ID': '1217:7134',
> +                'PCI_SUBSYS_ID': '10CF:131E',
> +                'PCI_SLOT_NAME': '0000:08:03.0',
> +                'SUBSYSTEM': 'pci',
> +                }
> +            }
> +
> +        self.pccard_scsi_controller_data = {
>              'P': '/devices/pci0000:00/0000:00:1e.0/0000:08:03.0/0000:09:00.0',
>              'E': {
>                  'DRIVER': 'aic7xxx',
> @@ -2857,6 +2914,12 @@
>                  },
>              }
>  
> +        self.pci_bridge_pccard_hierarchy_data = [
> +            {'udev_data': self.root_device},
> +            {'udev_data': self.pci_pccard_bridge},
> +            {'udev_data': self.pccard_scsi_controller_data},
> +            ]
> +
>          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'),
> @@ -2890,7 +2953,7 @@
>              }
>  
>          self.scsi_device_hierarchy_data = [
> -            {'udev_data': self.pci_scsi_controller_data},
> +            {'udev_data': self.pccard_scsi_controller_data},

Was this another problem with the test data? or something you updated
for an updated test?

>              {'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},
> @@ -2968,6 +3031,7 @@
>                  'PRODUCT': '1307/163/100',
>                  'TYPE': '0/0/0',
>                  'INTERFACE': '8/6/80',
> +                'DEVTYPE': 'usb_interface',

OK, and that's the error you highlighted in the MP.

>                  'SUBSYSTEM': 'usb',
>                  },
>              }
> @@ -3214,6 +3278,14 @@
>          device = UdevDevice(None, self.no_subsystem_device_data)
>          self.assertEqual(None, device.raw_bus)
>  
> +    def test_is_root_device(self):
> +        """Test of UdevDevice.is_root_device."""
> +        device = UdevDevice(None, self.root_device)
> +        self.assertTrue(device.is_root_device)
> +
> +        device = UdevDevice(None, self.pci_device_data)
> +        self.assertFalse(device.is_root_device)
> +
>      def test_getVendorOrProduct(self):
>          """Test of UdevDevice.getVendorOrProduct()."""
>          device = UdevDevice(
> @@ -3422,6 +3494,54 @@
>          device = UdevDevice(None, self.root_device)
>          self.assertEqual(None, device.translateScsiBus())
>  
> +    def test_translatePciBus(self):
> +        """Test of UdevDevice.translatePciBus()."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.pci_bridge_pccard_hierarchy_data)
> +        pci_device = devices[1]
> +        pccard_device = devices[2]
> +        self.assertEqual(HWBus.PCI, pci_device.translatePciBus())
> +        self.assertEqual(HWBus.PCCARD, pccard_device.translatePciBus())
> +
> +    def test_real_bus_usb_device(self):
> +        """Test of UdevDevice.real_bus for a USB device."""
> +        usb_device = UdevDevice(None, self.usb_device_data)
> +        self.assertEqual(HWBus.USB, usb_device.real_bus)
> +
> +    def test_real_bus_usb_interface(self):
> +        """Test of UdevDevice.real_bus for a USB interface."""
> +        parser = SubmissionParser(self.log)
> +        parser.submission_key = 'UdevDevice.real_bus for a not-real device'
> +        usb_interface = UdevDevice(parser, self.usb_storage_usb_interface)
> +        self.assertEqual(None, usb_interface.real_bus)
> +        # UdevDevice.real_bus should only be accessed for real devices,
> +        # which a USB is not. Hence we get a warning.
> +        self.assertWarningMessage(
> +            parser.submission_key,
> +            "Unknown bus 'usb_interface' for device "
> +            "/devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0")
> +
> +    def test_real_bus_pci(self):
> +        """Test of UdevDevice.real_bus for PCI devices."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.pci_bridge_pccard_hierarchy_data)
> +        pci_device = devices[1]
> +        pccard_device = devices[2]
> +        self.assertEqual(HWBus.PCI, pci_device.real_bus)
> +        self.assertEqual(HWBus.PCCARD, pccard_device.real_bus)
> +
> +    def test_real_bus_scsi(self):
> +        """Test of UdevDevice.real_bus for a SCSI device."""
> +        devices = self.buildUdevDeviceHierarchy(
> +            self.scsi_device_hierarchy_data)
> +        scsi_device = devices[-1]
> +        self.assertEqual(HWBus.SCSI, scsi_device.real_bus)
> +
> +    def test_real_bus_system(self):
> +        """Test of UdevDevice.real_bus for a system."""
> +        root_device = UdevDevice(None, self.root_device)
> +        self.assertEqual(HWBus.SYSTEM, root_device.real_bus)
> +
>  
>  class TestHWDBSubmissionTablePopulation(TestCaseHWDB):
>      """Tests of the HWDB popoluation with submitted data."""

-- 
Michael