Launchpad itself

Merge lp:~adeuring/launchpad/bug-458029-kernel-package-name-for-udev-submissions into lp:launchpad

  1. bug-458029-kernel-package-name-for-udev-submissions
  2. Merge into devel
Proposed by Abel Deuring
Status: Merged
Approved by: Muharem Hrnjadovic
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/bug-458029-kernel-package-name-for-udev-submissions
Merge into: lp:launchpad
Diff against target: 429 lines
3 files modified
lib/canonical/launchpad/scripts/hwdbsubmissions.py (+22/-10)
lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py (+3/-0)
lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py (+194/-36)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-458029-kernel-package-name-for-udev-submissions
Reviewer Review Type Date Requested Status
Muharem Hrnjadovic (community) Approve
Review via email: mp+13781@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :
Download full text (16.9 KiB)

This branch changes the method SubmissionParser.getKernelPackageName() into a property and makes it "aware" of submissions containig udev data instaed of HAL data.

One detail of the data about a device we store in the HWDB is a driver used to control a given device and the deb package of this driver. For kernel drivers, the package name is 'linux-image'+<kernel-version>.

Most submissions have two sources for the kernel version and the package name:

- The submitted data may contain the set of installed packages
- Submissions with HAL data, the root HAL device has a property 'system.kernel.version'; submissions with udev data have an XML node <summary><kernel-release>

Two source for this data mean that sone inconcistencies are possible; the method/proerty has some checks for these inconsistencies.

A problem with early versions of the processing was that getKernelPackageName is called for nearly each device of a submission (typically more 50), which led to a log file cluttered with 50 or more identical warnings, if there was a name inconsistency.

In a later version of the script, I introduced the optional parameter warning_id, which allow to suppress the repeated logging of warning with the same ID. The tests of getKernelPackageName/kernel_package_name ensure that a warning appears only once, even if the method/property is used more than once.

We have the nice feature of cached properties, so I declared the property as cachable, allowing to omit the warning_id parameter from the _logWarning call, and having the additional benefit of being a bit faster.

This branch is based on the reviewed but not yet merged branch lp:~adeuring/launchpad/bug-457475-udev-device-id; a diff of the changes in this branch is below.

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)

These complaints are not related to my changes.

=== modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
--- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-21 16:49:03 +0000
+++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-22 11:04:03 +0000
@@ -33,6 +33,7 @@

 from canonical.lazr.xml import RelaxNGValidator

+from canonical.cachedproperty import cachedproperty
 from canonical.config import config
 from canonical.librarian.interfaces import LibrarianServerError
 from canonical.launchpad.interfaces.hwdb import (
@@ -1470,15 +1471,20 @@
         del self.devices['/devices']
         return True

- def getKernelPackageName(self):
- """Return the kernel package name of the submission,"""
- root_hal_device = self.devices[ROOT_UDI]
- kern...

Revision history for this message
Muharem Hrnjadovic (al-maisan) wrote :

Hello Abel!

This looks good. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
--- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-21 10:06:31 +0000
+++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-22 11:50:26 +0000
@@ -33,6 +33,7 @@
3333
34from canonical.lazr.xml import RelaxNGValidator34from canonical.lazr.xml import RelaxNGValidator
3535
36from canonical.cachedproperty import cachedproperty
36from canonical.config import config37from canonical.config import config
37from canonical.librarian.interfaces import LibrarianServerError38from canonical.librarian.interfaces import LibrarianServerError
38from canonical.launchpad.interfaces.hwdb import (39from canonical.launchpad.interfaces.hwdb import (
@@ -503,6 +504,7 @@
503 devices = []504 devices = []
504 device = None505 device = None
505 line_number = 0506 line_number = 0
507 device_id = 0
506508
507 for line_number, line in enumerate(udev_data):509 for line_number, line in enumerate(udev_data):
508 if len(line) == 0:510 if len(line) == 0:
@@ -518,9 +520,11 @@
518520
519 key, value = record521 key, value = record
520 if device is None:522 if device is None:
523 device_id += 1
521 device = {524 device = {
522 'E': {},525 'E': {},
523 'S': [],526 'S': [],
527 'id': device_id,
524 }528 }
525 devices.append(device)529 devices.append(device)
526 # Some attribute lines have a space character after the530 # Some attribute lines have a space character after the
@@ -1467,15 +1471,20 @@
1467 del self.devices['/devices']1471 del self.devices['/devices']
1468 return True1472 return True
14691473
1470 def getKernelPackageName(self):1474 @cachedproperty
1471 """Return the kernel package name of the submission,"""1475 def kernel_package_name(self):
1472 root_hal_device = self.devices[ROOT_UDI]1476 """The kernel package name for the submission."""
1473 kernel_version = root_hal_device.getProperty('system.kernel.version')1477 if ROOT_UDI in self.devices:
1478 root_hal_device = self.devices[ROOT_UDI]
1479 kernel_version = root_hal_device.getProperty(
1480 'system.kernel.version')
1481 else:
1482 kernel_version = self.parsed_data['summary'].get('kernel-release')
1474 if kernel_version is None:1483 if kernel_version is None:
1475 self._logWarning(1484 self._logWarning(
1476 'Submission does not provide property system.kernel.version '1485 'Submission does not provide property system.kernel.version '
1477 'for /org/freedesktop/Hal/devices/computer.',1486 'for /org/freedesktop/Hal/devices/computer or a summary '
1478 WARNING_NO_HAL_KERNEL_VERSION)1487 'sub-node <kernel-release>.')
1479 return None1488 return None
1480 kernel_package_name = 'linux-image-' + kernel_version1489 kernel_package_name = 'linux-image-' + kernel_version
1481 packages = self.parsed_data['software']['packages']1490 packages = self.parsed_data['software']['packages']
@@ -1487,8 +1496,7 @@
1487 'Inconsistent kernel version data: According to HAL the '1496 'Inconsistent kernel version data: According to HAL the '
1488 'kernel is %s, but the submission does not know about a '1497 'kernel is %s, but the submission does not know about a '
1489 'kernel package %s'1498 'kernel package %s'
1490 % (kernel_version, kernel_package_name),1499 % (kernel_version, kernel_package_name))
1491 WARNING_NO_HAL_KERNEL_VERSION)
1492 return None1500 return None
1493 return kernel_package_name1501 return kernel_package_name
14941502
@@ -2182,10 +2190,9 @@
2182 # drivers, so there is currently no need to search for2190 # drivers, so there is currently no need to search for
2183 # for user space printer drivers, for example.2191 # for user space printer drivers, for example.
2184 if self.driver_name is not None:2192 if self.driver_name is not None:
2185 kernel_package_name = self.parser.getKernelPackageName()
2186 db_driver_set = getUtility(IHWDriverSet)2193 db_driver_set = getUtility(IHWDriverSet)
2187 return db_driver_set.getOrCreate(2194 return db_driver_set.getOrCreate(
2188 kernel_package_name, self.driver_name)2195 self.parser.kernel_package_name, self.driver_name)
2189 else:2196 else:
2190 return None2197 return None
21912198
@@ -2776,6 +2783,11 @@
2776 return None2783 return None
2777 return controller2784 return controller
27782785
2786 @property
2787 def id(self):
2788 return self.udev['id']
2789
2790
2779class ProcessingLoop(object):2791class ProcessingLoop(object):
2780 """An `ITunableLoop` for processing HWDB submissions."""2792 """An `ITunableLoop` for processing HWDB submissions."""
27812793
27822794
=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py'
--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py 2009-10-12 14:50:22 +0000
+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_parser.py 2009-10-22 11:50:26 +0000
@@ -635,6 +635,7 @@
635 'MODALIAS': 'acpi:LNXSYSTM:',635 'MODALIAS': 'acpi:LNXSYSTM:',
636 },636 },
637 'S': [],637 'S': [],
638 'id': 1,
638 },639 },
639 {640 {
640 'P': '/devices/pci0000:00/0000:00:1a.0',641 'P': '/devices/pci0000:00/0000:00:1a.0',
@@ -643,6 +644,7 @@
643 'DEVPATH': '/devices/pci0000:00/0000:00:1a.0',644 'DEVPATH': '/devices/pci0000:00/0000:00:1a.0',
644 },645 },
645 'S': ['char/189:256'],646 'S': ['char/189:256'],
647 'id': 2,
646 },648 },
647 ],649 ],
648 result,650 result,
@@ -704,6 +706,7 @@
704 'E': {},706 'E': {},
705 'S': [],707 'S': [],
706 'W': '2',708 'W': '2',
709 'id': 1,
707 },710 },
708 ],711 ],
709 result,712 result,
710713
=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-21 10:06:31 +0000
+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-22 11:50:26 +0000
@@ -360,8 +360,8 @@
360 self.assertErrorMessage(360 self.assertErrorMessage(
361 parser.submission_key, "No udev root device defined")361 parser.submission_key, "No udev root device defined")
362362
363 def testKernelPackageName(self):363 def test_kernel_package_name_hal_data(self):
364 """Test of SubmissionParser.getKernelPackageName.364 """Test of SubmissionParser.kernel_package_name.
365365
366 Regular case.366 Regular case.
367 """367 """
@@ -388,17 +388,19 @@
388 },388 },
389 }389 }
390 parser.buildDeviceList(parser.parsed_data)390 parser.buildDeviceList(parser.parsed_data)
391 kernel_package = parser.getKernelPackageName()391 kernel_package = parser.kernel_package_name
392 self.assertEqual(kernel_package, self.KERNEL_PACKAGE,392 self.assertEqual(
393 'Unexpected result of SubmissionParser.getKernelPackageName. '393 self.KERNEL_PACKAGE, kernel_package,
394 'Unexpected value of SubmissionParser.kernel_package_name. '
394 'Expected linux-image-2.6.24-19-generic, got %r' % kernel_package)395 'Expected linux-image-2.6.24-19-generic, got %r' % kernel_package)
395396
396 self.assertEqual(len(self.handler.records), 0,397 self.assertEqual(
398 0, len(self.handler.records),
397 'One or more warning messages were logged by '399 'One or more warning messages were logged by '
398 'getKernelPackageName, where zero was expected.')400 'SubmissionParser.kernel_package_name, where zero was expected.')
399401
400 def testKernelPackageNameInconsistent(self):402 def test_kernel_package_hal_data_name_inconsistent(self):
401 """Test of SubmissionParser.getKernelPackageName.403 """Test of SubmissionParser.kernel_package_name.
402404
403 Test a name inconsistency.405 Test a name inconsistency.
404 """406 """
@@ -426,24 +428,24 @@
426 }428 }
427 parser.submission_key = 'Test of inconsistent kernel package name'429 parser.submission_key = 'Test of inconsistent kernel package name'
428 parser.buildDeviceList(parser.parsed_data)430 parser.buildDeviceList(parser.parsed_data)
429 kernel_package = parser.getKernelPackageName()431 kernel_package = parser.kernel_package_name
430 self.assertEqual(kernel_package, None,432 self.assertIs(None, kernel_package)
431 'Unexpected result of SubmissionParser.getKernelPackageName. '433 self.assertWarningMessage(
432 'Expected None, got %r' % kernel_package)434 parser.submission_key,
433 self.assertWarningMessage(parser.submission_key,
434 'Inconsistent kernel version data: According to HAL the '435 'Inconsistent kernel version data: According to HAL the '
435 'kernel is 2.6.24-19-generic, but the submission does not '436 'kernel is 2.6.24-19-generic, but the submission does not '
436 'know about a kernel package linux-image-2.6.24-19-generic')437 'know about a kernel package linux-image-2.6.24-19-generic')
437 # The warning appears only once per submission, even if the438 # The warning appears only once per submission, even if the
438 # SubmissionParser.getKernelPackageName is called more than once.439 # property kernel_package_name is accessed more than once.
439 num_warnings = len(self.handler.records)440 num_warnings = len(self.handler.records)
440 parser.getKernelPackageName()441 test = parser.kernel_package_name
441 self.assertEqual(num_warnings, len(self.handler.records),442 self.assertEqual(
443 num_warnings, len(self.handler.records),
442 'Warning for missing HAL property system.kernel.version '444 'Warning for missing HAL property system.kernel.version '
443 'repeated.')445 'repeated.')
444446
445 def testKernelPackageNameNoHALData(self):447 def test_kernel_package_name_hal_data_no_kernel_version_in_hal_data(self):
446 """Test of SubmissionParser.getKernelPackageName.448 """Test of SubmissionParser.kernel_package_name.
447449
448 Test without HAL property system.kernel.version.450 Test without HAL property system.kernel.version.
449 """451 """
@@ -469,26 +471,28 @@
469 }471 }
470 parser.submission_key = 'Test: missing property system.kernel.version'472 parser.submission_key = 'Test: missing property system.kernel.version'
471 parser.buildDeviceList(parser.parsed_data)473 parser.buildDeviceList(parser.parsed_data)
472 kernel_package = parser.getKernelPackageName()474 self.assertIs(None, parser.kernel_package_name)
473 self.assertEqual(kernel_package, None,475 self.assertWarningMessage(
474 'Unexpected result of SubmissionParser.getKernelPackageName. '476 parser.submission_key,
475 'Expected None, got %r' % kernel_package)
476 self.assertWarningMessage(parser.submission_key,
477 'Submission does not provide property system.kernel.version '477 'Submission does not provide property system.kernel.version '
478 'for /org/freedesktop/Hal/devices/computer.')478 'for /org/freedesktop/Hal/devices/computer or a summary '
479 'sub-node <kernel-release>.')
479 # The warning appears only once per submission, even if the480 # The warning appears only once per submission, even if the
480 # SubmissionParser.getKernelPackageName is called more than once.481 # property kernel_package_name is accessed more than once.
481 num_warnings = len(self.handler.records)482 num_warnings = len(self.handler.records)
482 parser.getKernelPackageName()483 test = parser.kernel_package_name
483 self.assertEqual(num_warnings, len(self.handler.records),484 self.assertEqual(
485 num_warnings, len(self.handler.records),
484 'Warning for missing HAL property system.kernel.version '486 'Warning for missing HAL property system.kernel.version '
485 'repeated.')487 'repeated.')
486488
487 def testKernelPackageNameNoPackageData(self):489 def test_kernel_package_name_hal_data_no_package_data(self):
488 """Test of SubmissionParser.getKernelPackageName.490 """Test of SubmissionParser.kernel_package_name.
489491
490 Test without any package data. getKernelPackageName returns492 Test without any package data. In this case,
491 the property system.kernel.version without any further checking.493 SubmissionParser.kernel_package_name is the value of the property
494 system.kernel.version if the root HAL device. No further checks
495 are done.
492 """496 """
493 parser = SubmissionParser(self.log)497 parser = SubmissionParser(self.log)
494 devices = [498 devices = [
@@ -512,8 +516,156 @@
512 }516 }
513 parser.submission_key = 'Test: missing property system.kernel.version'517 parser.submission_key = 'Test: missing property system.kernel.version'
514 parser.buildDeviceList(parser.parsed_data)518 parser.buildDeviceList(parser.parsed_data)
515 kernel_package = parser.getKernelPackageName()519 kernel_package = parser.kernel_package_name
516 self.assertEqual(kernel_package, self.KERNEL_PACKAGE,520 self.assertEqual(
521 self.KERNEL_PACKAGE, kernel_package,
522 'Unexpected result of SubmissionParser.getKernelPackageName, '
523 'test without any package data. Expected None, got %r'
524 % kernel_package)
525
526 def test_kernel_package_name_udev_data(self):
527 """Test of SubmissionParser.kernel_package_name for udev data.
528
529 Variant for udev data, regular case.
530 """
531 parser = SubmissionParser(self.log)
532 parser.parsed_data = {
533 'hardware': {
534 'udev': [
535 {'P': '/devices/LNXSYSTM:00'}
536 ],
537 'sysfs-attributes': {},
538 'dmi': {},
539 },
540 'software': {
541 'packages': {
542 self.KERNEL_PACKAGE: {},
543 },
544 },
545 'summary': {
546 'kernel-release': self.KERNEL_VERSION,
547 },
548 }
549 parser.buildDeviceList(parser.parsed_data)
550 kernel_package = parser.kernel_package_name
551 self.assertEqual(
552 self.KERNEL_PACKAGE, kernel_package,
553 'Unexpected value of SubmissionParser.kernel_package_name. '
554 'Expected linux-image-2.6.24-19-generic, got %r' % kernel_package)
555
556 self.assertEqual(
557 0, len(self.handler.records),
558 'One or more warning messages were logged by '
559 'SubmissionParser.kernel_package_name, where zero was expected.')
560
561 def test_kernel_package_udev_data_name_inconsistent(self):
562 """Test of SubmissionParser.kernel_package_name.
563
564 Variant for udev data, name inconsistency.
565 """
566 parser = SubmissionParser(self.log)
567 parser.parsed_data = {
568 'hardware': {
569 'udev': [
570 {'P': '/devices/LNXSYSTM:00'}
571 ],
572 'sysfs-attributes': {},
573 'dmi': {},
574 },
575 'software': {
576 'packages': {
577 'linux-image-from-obscure-external-source': {},
578 },
579 },
580 'summary': {
581 'kernel-release': self.KERNEL_VERSION,
582 },
583 }
584 parser.submission_key = 'Test of inconsistent kernel package name'
585 parser.buildDeviceList(parser.parsed_data)
586 kernel_package = parser.kernel_package_name
587 self.assertIs(None, kernel_package)
588 self.assertWarningMessage(
589 parser.submission_key,
590 'Inconsistent kernel version data: According to HAL the '
591 'kernel is 2.6.24-19-generic, but the submission does not '
592 'know about a kernel package linux-image-2.6.24-19-generic')
593 # The warning appears only once per submission, even if the
594 # property kernel_package_name is accessed more than once.
595 num_warnings = len(self.handler.records)
596 test = parser.kernel_package_name
597 self.assertEqual(
598 num_warnings, len(self.handler.records),
599 'Warning for missing HAL property system.kernel.version '
600 'repeated.')
601
602 def test_kernel_package_name_udev_data_no_kernel_version_in_summary(self):
603 """Test of SubmissionParser.kernel_package_name.
604
605 Test without the summary sub-node <kernel-release>.
606 """
607 parser = SubmissionParser(self.log)
608 parser.parsed_data = {
609 'hardware': {
610 'udev': [
611 {'P': '/devices/LNXSYSTM:00'}
612 ],
613 'sysfs-attributes': {},
614 'dmi': {},
615 },
616 'software': {
617 'packages': {
618 self.KERNEL_PACKAGE: {},
619 },
620 },
621 'summary': {},
622 }
623 parser.submission_key = 'Test: missing property system.kernel.version'
624 parser.buildDeviceList(parser.parsed_data)
625 self.assertIs(None, parser.kernel_package_name)
626 self.assertWarningMessage(
627 parser.submission_key,
628 'Submission does not provide property system.kernel.version '
629 'for /org/freedesktop/Hal/devices/computer or a summary '
630 'sub-node <kernel-release>.')
631 # The warning appears only once per submission, even if the
632 # property kernel_package_name is accessed more than once.
633 num_warnings = len(self.handler.records)
634 test = parser.kernel_package_name
635 self.assertEqual(
636 num_warnings, len(self.handler.records),
637 'Warning for missing HAL property system.kernel.version '
638 'repeated.')
639
640 def test_kernel_package_name_udev_data_no_package_data(self):
641 """Test of SubmissionParser.kernel_package_name.
642
643 Variant for udev data, test without any package data. In this case,
644 SubmissionParser.kernel_package_name is the value of the property
645 system.kernel.version if the root HAL device. No further checks
646 are done.
647 """
648 parser = SubmissionParser(self.log)
649 parser.parsed_data = {
650 'hardware': {
651 'udev': [
652 {'P': '/devices/LNXSYSTM:00'},
653 ],
654 'sysfs-attributes': {},
655 'dmi': {},
656 },
657 'software': {
658 'packages': {},
659 },
660 'summary': {
661 'kernel-release': self.KERNEL_VERSION,
662 },
663 }
664 parser.submission_key = 'Test: missing property system.kernel.version'
665 parser.buildDeviceList(parser.parsed_data)
666 kernel_package = parser.kernel_package_name
667 self.assertEqual(
668 self.KERNEL_PACKAGE, kernel_package,
517 'Unexpected result of SubmissionParser.getKernelPackageName, '669 'Unexpected result of SubmissionParser.getKernelPackageName, '
518 'test without any package data. Expected None, got %r'670 'test without any package data. Expected None, got %r'
519 % kernel_package)671 % kernel_package)
@@ -3036,7 +3188,8 @@
3036 'DEVPATH': '/devices/LNXSYSTM:00',3188 'DEVPATH': '/devices/LNXSYSTM:00',
3037 'MODALIAS': 'acpi:LNXSYSTM:',3189 'MODALIAS': 'acpi:LNXSYSTM:',
3038 'SUBSYSTEM': 'acpi',3190 'SUBSYSTEM': 'acpi',
3039 }3191 },
3192 'id': 1,
3040 }3193 }
30413194
3042 self.root_device_dmi_data = {3195 self.root_device_dmi_data = {
@@ -3520,7 +3673,7 @@
3520 },3673 },
3521 }3674 }
35223675
3523 def test_device_id(self):3676 def test_device_device_id(self):
3524 """Test of UdevDevice.device_id."""3677 """Test of UdevDevice.device_id."""
3525 device = UdevDevice(None, self.pci_sata_controller)3678 device = UdevDevice(None, self.pci_sata_controller)
3526 self.assertEqual(3679 self.assertEqual(
@@ -4185,6 +4338,11 @@
4185 'provide bus, vendor ID, product ID or product name: None None '4338 'provide bus, vendor ID, product ID or product name: None None '
4186 'None None /devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0')4339 'None None /devices/pci0000:00/0000:00:1d.7/usb1/1-1/1-1:1.0')
41874340
4341 def test_device_id(self):
4342 """Each UdevDevice has a property 'id'."""
4343 device = UdevDevice(None, self.root_device)
4344 self.assertEqual(1, device.id)
4345
41884346
4189class TestHWDBSubmissionTablePopulation(TestCaseHWDB):4347class TestHWDBSubmissionTablePopulation(TestCaseHWDB):
4190 """Tests of the HWDB popoluation with submitted data."""4348 """Tests of the HWDB popoluation with submitted data."""