Launchpad itself

Code review comment for lp:~adeuring/launchpad/hwdb-class-udev-device-7

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

Revision history for this message

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

Hi Michael,

thanks for your review!

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

right, fixed.

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

I changed the name because the device is indeed a PCCard, an I use it also in a new test of translatePciBus(), so I thought that the test data should get a more precise name.

Abel

@@ -1173,7 +1173,7 @@
parser.submission_key,
"Unknown bus 'nonsense' for device " + self.UDI_PCCARD_DEVICE)

- def testHALDevice_is_root_device_for_root_device(self):
+ def test_HALDevice_is_root_device_for_root_device(self):
         """Test of HALDevice.is_root_device for the root device."""
         devices = [
             {
@@ -1195,7 +1195,7 @@
         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):
+ def test_HALDevice_is_root_device_for_non_root_device(self):
         """Test of HALDevice.is_root_device for a non-root device."""
         devices = [
             {

Hi Michael,

thanks for your review!

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

right, fixed.

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

I changed the name because the device is indeed a PCCard, an I use it also in a new test of translatePciBus(), so I thought that the test data should get a more precise name.

Abel

@@ -1173,7 +1173,7 @@
             parser.submission_key,
             "Unknown bus 'nonsense' for device " + self.UDI_PCCARD_DEVICE)

-    def testHALDevice_is_root_device_for_root_device(self):
+    def test_HALDevice_is_root_device_for_root_device(self):
         """Test of HALDevice.is_root_device for the root device."""
         devices = [
             {
@@ -1195,7 +1195,7 @@
         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):
+    def test_HALDevice_is_root_device_for_non_root_device(self):
         """Test of HALDevice.is_root_device for a non-root device."""
         devices = [
             {

« Back to merge proposal