Hi Abel, A rather terse review from me today, since I'm also the CHR. I have some comments that should be easy to address. r=me, merge-conditional with their consideration. review approve status approve -Barry === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py' --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-08 13:41:32 +0000 +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-09 12:10:24 +0000 > @@ -1212,11 +1212,83 @@ > return False > return True > > + USB_DEVICE_PROPERTIES = set(('DEVTYPE', 'PRODUCT', 'TYPE')) > + usb_product_re = re.compile( > + '^[0-9a-f]{1,4}/[0-9a-f]{1,4}/[0-9a-f]{1,4}$', re.I) > + usb_type_re = re.compile('^[0-9]{1,3}/[0-9]{1,3}/[0-9]{1,3}$') Is there a reason these are class attributes? They probably make more sense being module globals, with all-caps names. > + > + def checkUdevUsbProperties(self, udev_data): > + """Validation of udev USB devices. > + > + USB devices must have the properties DEVTYPE (value > + 'usb_device' or 'usb_interface'), PRODUCT and TYPE. PRODUCT > + must be a tuple of three integers in hexadecimal > + representation, separates by '/'. TYPE must be a a tuple of > + three integers in decimal representation, separated by '/'. > + usb_interface nodes must additionally have a property > + INTERFACE, containing three integers in the same format as > + TYPE. > + """ > + for device in udev_data: > + subsystem = device['E'].get('SUBSYSTEM') > + if subsystem != 'usb': > + continue > + properties = device['E'] > + property_names = set(properties.keys()) Since 'properties' is a dictionary, this is more efficient: property_names = set(properties) > + existing_usb_properties = property_names.intersection( > + self.USB_DEVICE_PROPERTIES) > + if existing_usb_properties != self.USB_DEVICE_PROPERTIES: > + self._logError( > + 'USB udev device found without required properties: %r %r' > + % (self.USB_DEVICE_PROPERTIES.difference( > + existing_usb_properties), > + device['P']), This is somewhat unreadable. You should move the .difference() calculation to above the self._logError() call and stash it in a local variable, then use the local variable in the interpolation. > + self.submission_key) > + return False > + if self.usb_product_re.search(properties['PRODUCT']) is None: > + self._logError( > + 'USB udev device found with invalid product ID: %r %r' > + % (properties['PRODUCT'], device['P']), > + self.submission_key) > + return False > + if self.usb_type_re.search(properties['TYPE']) is None: > + self._logError( > + 'USB udev device found with invalid type data: %r %r' > + % (properties['TYPE'], device['P']), > + self.submission_key) > + return False > + > + device_type = properties['DEVTYPE'] > + if device_type not in ('usb_device', 'usb_interface'): > + self._logError( > + 'USB udev device found with invalid udev type data: %r %r' > + % (device_type, device['P']), > + self.submission_key) > + return False > + if device_type == 'usb_interface': > + interface_type = properties.get('INTERFACE') > + if interface_type is None: > + self._logError( > + 'USB interface udev device found without INTERFACE ' > + 'property: %r' > + % device['P'], > + self.submission_key) > + return False > + if self.usb_type_re.search(interface_type) is None: > + self._logError( > + 'USB Interface udev device found with invalid ' > + 'INTERFACE property: %r %r' > + % (interface_type, device['P']), > + self.submission_key) > + return False > + return True > + > def checkConsistentUdevDeviceData(self, udev_data): > """Consistency checks for udev data.""" > - if not self.checkUdevDictsHavePathKey(udev_data): > - return False > - return self.checkUdevPciProperties(udev_data) > + return ( > + self.checkUdevDictsHavePathKey(udev_data) and > + self.checkUdevPciProperties(udev_data) and > + self.checkUdevUsbProperties(udev_data)) > > def checkConsistency(self, parsed_data): > """Run consistency checks on the submitted data. > @@ -2278,6 +2350,39 @@ > """See `BaseDevice`.""" > return self.pci_class_info[1] > > + @property > + def is_usb(self): > + """True, if this is a USB device, else False.""" > + return self.udev['E'].get('SUBSYSTEM') == 'usb' > + > + @property > + def usb_ids(self): > + """The vendor ID, product ID, product version for USB devices. > + > + :return: [vendor_id, product_id, version] for USB devices > + or [None, None, None] for other devices. > + """ > + if self.is_usb: > + # udev represents USB device IDs as strings > + # vendor_id/prodct_id/version, where each part is > + # as a hexdecimal number. > + # SubmissionParser.checkUdevUsbProperties() ensures that > + # the string PRODUCT is in the format required below. > + product_info = self.udev['E']['PRODUCT'].split('/') > + return [int(part, 16) for part in product_info] > + else: > + return [None, None, None] > + > + @property > + def usb_vendor_id(self): > + """See `BaseDevice`.""" > + return self.usb_ids[0] > + > + @property > + def usb_product_id(self): > + """See `BaseDevice`.""" > + return self.usb_ids[1] > + > > class ProcessingLoop(object): > """An `ITunableLoop` for processing HWDB submissions.""" === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py' --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py 2009-10-08 13:41:32 +0000 +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py 2009-10-09 12:10:24 +0000 > @@ -112,6 +112,25 @@ > 'PCI_SLOT_NAME': '0000:00:1f.2', > } > } > + self.udev_usb_device = { > + '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', > + }, > + } > + self.udev_usb_interface = { > + 'P': '/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.1', > + 'E': { > + 'SUBSYSTEM': 'usb', > + 'DEVTYPE': 'usb_interface', > + 'PRODUCT': '46d/a01/1013', > + 'TYPE': '0/0/0', > + 'INTERFACE': '1/2/0', > + }, > + } > > def getTimestampETreeNode(self, time_string): > """Return an Elementtree node for an XML tag with a timestamp.""" > @@ -1788,7 +1807,7 @@ > parser.submission_key, 'udev node found without a "P" key') > > def testCheckUdevPciProperties(self): > - """Test of SubmmissionParser.checkUdevPciProperties().""" > + """Test of SubmissionParser.checkUdevPciProperties().""" > # udev PCI devices must have the properties PCI_CLASS, PCI_ID, > # PCI_SUBSYS_ID, PCI_SLOT_NAME; other devices must not have > # these properties. > @@ -1797,7 +1816,7 @@ > [self.udev_root_device, self.udev_pci_device])) > > def testCheckUdevPciPropertiesNonPciDeviceWithPciProperties(self): > - """Test of SubmmissionParser.checkUdevPciProperties(). > + """Test of SubmissionParser.checkUdevPciProperties(). > > A non-PCI device having PCI properties makes a submission invalid. > """ > @@ -1812,7 +1831,7 @@ > "'/devices/LNXSYSTM:00'") > > def testCheckUdevPciPropertiesPciDeviceWithoutRequiredProperties(self): > - """Test of SubmmissionParser.checkUdevPciProperties(). > + """Test of SubmissionParser.checkUdevPciProperties(). > > A PCI device not having a required PCI property makes a submission > invalid. > @@ -1828,7 +1847,7 @@ > "set(['PCI_CLASS']) '/devices/pci0000:00/0000:00:1f.2'") > > def testCheckUdevPciPropertiesPciDeviceWithNonIntegerPciClass(self): > - """Test of SubmmissionParser.checkUdevPciProperties(). > + """Test of SubmissionParser.checkUdevPciProperties(). > > A PCI device with a non-integer class value makes a submission > invalid. > @@ -1844,7 +1863,7 @@ > "'/devices/pci0000:00/0000:00:1f.2'") > > def testCheckUdevPciPropertiesPciDeviceWithInvalidPciClassValue(self): > - """Test of SubmmissionParser.checkUdevPciProperties(). > + """Test of SubmissionParser.checkUdevPciProperties(). > > A PCI device with invalid class data makes a submission > invalid. > @@ -1860,7 +1879,7 @@ > "'/devices/pci0000:00/0000:00:1f.2'") > > def testCheckUdevPciPropertiesPciDeviceWithInvalidDeviceID(self): > - """Test of SubmmissionParser.checkUdevPciProperties(). > + """Test of SubmissionParser.checkUdevPciProperties(). > > A PCI device with an invalid device ID makes a submission > invalid. > @@ -1876,7 +1895,7 @@ > "'/devices/pci0000:00/0000:00:1f.2'") > > def testCheckUdevPciPropertiesPciDeviceWithInvalidSubsystemID(self): > - """Test of SubmmissionParser.checkUdevPciProperties(). > + """Test of SubmissionParser.checkUdevPciProperties(). > > A PCI device with an invalid subsystem ID makes a submission > invalid. > @@ -1891,6 +1910,110 @@ > "Invalid udev PCI device ID: 'not-a-subsystem-id' " > "'/devices/pci0000:00/0000:00:1f.2'") > > + def testCheckUdevUsbProperties(self): > + """Test of SubmissionParser.checkUdevUsbProperties().""" > + parser = SubmissionParser() > + self.assertTrue(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_device, > + self.udev_usb_interface])) > + > + def testCheckUdevUsbProperties_missing_required_property(self): > + """Test of SubmissionParser.checkUdevUsbProperties(). > + > + A USB device that does not have a required property makes a > + submission invalid. > + """ > + del self.udev_usb_device['E']['DEVTYPE'] > + parser = SubmissionParser(self.log) > + parser.submission_key = 'USB device without DEVTYPE property' > + self.assertFalse(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_device])) > + self.assertErrorMessage( > + parser.submission_key, > + "USB udev device found without required properties: " > + "set(['DEVTYPE']) '/devices/pci0000:00/0000:00:1d.1/usb3/3-2'") > + > + def testCheckUdevUsbProperties_with_invalid_product_id(self): > + """Test of SubmissionParser.checkUdevUsbProperties(). > + > + A USB device with an invalid product ID makes a submission > + invalid. > + """ > + self.udev_usb_device['E']['PRODUCT'] = 'not-a-valid-usb-product-id' > + parser = SubmissionParser(self.log) > + parser.submission_key = 'USB device with invalid product ID' > + self.assertFalse(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_device])) > + self.assertErrorMessage( > + parser.submission_key, > + "USB udev device found with invalid product ID: " > + "'not-a-valid-usb-product-id' " > + "'/devices/pci0000:00/0000:00:1d.1/usb3/3-2'") > + > + def testCheckUdevUsbProperties_with_invalid_type_data(self): > + """Test of SubmmissionParser.checkUdevUsbProperties(). > + > + A USB device with invalid type data makes a submission invalid. > + """ > + self.udev_usb_device['E']['TYPE'] = 'no-type' > + parser = SubmissionParser(self.log) > + parser.submission_key = 'USB device with invalid type data' > + self.assertFalse(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_device])) > + self.assertErrorMessage( > + parser.submission_key, > + "USB udev device found with invalid type data: 'no-type' " > + "'/devices/pci0000:00/0000:00:1d.1/usb3/3-2'") > + > + def testCheckUdevUsbProperties_with_invalid_devtype(self): > + """Test of SubmmissionParser.checkUdevUsbProperties(). > + > + A udev USB device must have DEVTYPE set to 'usb_device' or > + 'usb_interface'. > + """ > + self.udev_usb_device['E']['DEVTYPE'] = 'nonsense' > + parser = SubmissionParser(self.log) > + parser.submission_key = 'USB device with invalid DEVTYPE' > + self.assertFalse(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_device])) > + self.assertErrorMessage( > + parser.submission_key, > + "USB udev device found with invalid udev type data: 'nonsense' " > + "'/devices/pci0000:00/0000:00:1d.1/usb3/3-2'") > + > + def testCheckUdevUsbProperties_interface_without_interface_property(self): > + """Test of SubmmissionParser.checkUdevUsbProperties(). > + > + A udev USB device for a USB interface have the property INTERFACE. > + """ > + del self.udev_usb_interface['E']['INTERFACE'] > + parser = SubmissionParser(self.log) > + parser.submission_key = 'USB interface without INTERFACE property' > + self.assertFalse(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_interface])) > + self.assertErrorMessage( > + parser.submission_key, > + "USB interface udev device found without INTERFACE property: " > + "'/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.1'") > + > + def testCheckUdevUsbProperties_interface_invalid_interface_property(self): > + """Test of SubmmissionParser.checkUdevUsbProperties(). > + > + The INTERFACE proeprty of A udev USB device for a USB interface > + must have value in the format main_class/sub_class/version > + """ > + self.udev_usb_interface['E']['INTERFACE'] = 'nonsense' > + parser = SubmissionParser(self.log) > + parser.submission_key = 'USB interface with invalid INTERFACE data' > + self.assertFalse(parser.checkUdevUsbProperties( > + [self.udev_root_device, self.udev_usb_interface])) > + self.assertErrorMessage( > + parser.submission_key, > + "USB Interface udev device found with invalid INTERFACE " > + "property: 'nonsense' " > + "'/devices/pci0000:00/0000:00:1d.1/usb3/3-2/3-2:1.1'") > + > + > class UdevTestSubmissionParser(SubmissionParser): > """A variant of SubmissionParser that shortcuts udev related tests. > > @@ -1904,6 +2027,10 @@ > """See `SubmissionParser`.""" > return True > > + def checkUdevUsbProperties(self, udev_data): > + """See `SubmissionParser`.""" > + return True > + > def testCheckConsistentUdevDeviceData(self): > """Test of SubmissionParser.checkConsistentUdevDeviceData(),""" > parser = self.UdevTestSubmissionParser() > @@ -1941,6 +2068,22 @@ > parser = SubmissionParserUdevPciCheckFails() > self.assertFalse(parser.checkConsistentUdevDeviceData(None)) > > + def testCheckConsistentUdevDeviceData_invalid_usb_data(self): > + """Test of SubmissionParser.checkConsistentUdevDeviceData(), > + > + Detection of invalid PCI data lets the check fail. > + """ > + class SubmissionParserUdevUsbCheckFails( > + self.UdevTestSubmissionParser): > + """A SubmissionPaser where checkUdevPciProperties() fails.""" > + > + def checkUdevUsbProperties(self, udev_data): > + """See `SubmissionParser`.""" > + return False > + > + parser = SubmissionParserUdevUsbCheckFails() > + self.assertFalse(parser.checkConsistentUdevDeviceData(None)) > + > def _setupConsistencyCheckParser(self): > """Prepare and return a SubmissionParser instance. === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py' --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-08 13:41:32 +0000 +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-09 12:10:24 +0000 > @@ -2617,6 +2617,16 @@ > } > } > > + 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', > + }, > + } > + > def test_device_id(self): > """Test of UdevDevice.device_id.""" > device = UdevDevice(self.pci_device_data, None, None) > @@ -2668,6 +2678,50 @@ > None, device.pci_class, > 'Invalid value of UdevDevice.pci_class for Non-PCI device.') > > + def test_is_usb(self): > + """Test of UdevDevice.is_usb""" > + device = UdevDevice(self.usb_device_data, None, None) > + self.assertTrue(device.is_usb) > + > + device = UdevDevice(self.pci_device_data, None, None) > + self.assertFalse(device.is_usb) > + > + def test_usb_ids(self): > + """Test of UdevDevice.usb_ids""" > + device = UdevDevice(self.usb_device_data, None, None) > + self.assertEqual( > + [0x46d, 0xa01, 0x1013], device.usb_ids, > + 'Invalid value of UdevDevice.usb_ids for USB device.') > + > + device = UdevDevice(self.root_device, None, None) > + self.assertEqual( > + [None, None, None], device.usb_ids, > + 'Invalid value of UdevDevice.usb_ids for Non-USB device.') > + > + def test_usb_vendor_id(self): > + """Test of UdevDevice.usb_vendor_id""" > + device = UdevDevice(self.usb_device_data, None, None) > + self.assertEqual( > + 0x46d, device.usb_vendor_id, > + 'Invalid value of UdevDevice.usb_vendor_id for USB device.') > + > + device = UdevDevice(self.root_device, None, None) > + self.assertEqual( > + None, device.usb_vendor_id, > + 'Invalid value of UdevDevice.usb_vendor_id for Non-USB device.') > + > + def test_usb_product_id(self): > + """Test of UdevDevice.usb_product_id""" > + device = UdevDevice(self.usb_device_data, None, None) > + self.assertEqual( > + 0xa01, device.usb_product_id, > + 'Invalid value of UdevDevice.usb_product_id for USB device.') > + > + device = UdevDevice(self.root_device, None, None) > + self.assertEqual( > + None, device.usb_product_id, > + 'Invalid value of UdevDevice.usb_product_id for Non-USB device.') > + > > class TestHWDBSubmissionTablePopulation(TestCaseHWDB): > """Tests of the HWDB popoluation with submitted data."""