Launchpad itself

Merge lp:~adeuring/launchpad/hwdb-refactor-haldevice into lp:launchpad

  1. hwdb-refactor-haldevice
  2. Merge into devel
Proposed by Abel Deuring
Status: Merged
Merged at revision: not available
Proposed branch: lp:~adeuring/launchpad/hwdb-refactor-haldevice
Merge into: lp:launchpad
Diff against target: 315 lines
2 files modified
lib/canonical/launchpad/scripts/hwdbsubmissions.py (+107/-68)
lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py (+45/-0)
To merge this branch: bzr merge lp:~adeuring/launchpad/hwdb-refactor-haldevice
Reviewer Review Type Date Requested Status
Michael Nelson (community) code Approve
Review via email: mp+12669@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

This branch is a first step to refactor class HALDevice in l/c/l/scripts/hwdbsubmissions.py

The HWDB client in Karmic will no longer provide data gathrerd from HAL; instead it collects data from udev, sysfs attributes and some DMI data.

class HALDevice represents a device in the script that processes HWDB submissions; its purpose is to populate the HWDB tables with data from a submission. It relies heavily on many details how HAL represents a device; especially, it gathers all data from HAL properties (method HALDevice.getProperty()). The data provided by udev etc. is essentially quite similar, but has a different representation. For example, the PCI vendor and product ID is represented in HAL by the properties pci.vendor_id/pci.product_id, while udev provides them in a comon attribute called PCI_ID.

Especially the heuristics in HALDevice, for example, to figure out if a given HAL device represents a real physical device or only a specific "aspect" of a device, can stay the same for a (not yet existing) class UDevDevice. Similary, the code to populate the HWDB tables does not need to be duplicated for UDevDevice.

Hence it makes sense to move the common code of the classes HALDevice and UDevdevice into a common subclass, called BaseDevice.

BaseDevice defines up to now the new properties device_id, pci_class, pci_subclass, and it contains methods translateScsiBus() and translatePciBus(), both methods are moved from HALDevice. These methods called before the HAL-specific method getProperty(), and they accessed the HAL-specific attribute HALDevice.udi; this is replaced by the new properties.

test: ./bin/test -t 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 ==

These complaints are not related to the changes in this branch.

Revision history for this message
Michael Nelson (michael.nelson) wrote :
Download full text (4.6 KiB)

> This branch is a first step to refactor class HALDevice in
> l/c/l/scripts/hwdbsubmissions.py
>
...

r=me, just a few small copy-n-paste/ordering issues below.

Thanks for the detailed description! Makes much more sense now :) It's
nice seeing the preparation for the new UDevDevice in a separate small
branch!

> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-09-29 15:05:09 +0000
> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-01 07:42:22 +0000
> @@ -7,7 +7,7 @@
> data and for the community test submissions.
> """
>
> -
> +__metaclass__ = type
> __all__ = [
> 'SubmissionParser',
> 'process_pending_submissions',
> @@ -1235,59 +1235,29 @@
> root_device.createDBData(submission, None)
> return True
>
> -class HALDevice:
> - """The representation of a HAL device node."""
> -
> - def __init__(self, id, udi, properties, parser):
> - """HALDevice constructor.
> -
> - :param id: The ID of the HAL device in the submission data as
> - specified in <device id=...>.
> - :type id: int
> - :param udi: The UDI of the HAL device.
> - :type udi: string
> - :param properties: The HAL properties of the device.
> - :type properties: dict
> - :param parser: The parser processing a submission.
> - :type parser: SubmissionParser
> - """
> - self.id = id
> - self.udi = udi
> - self.properties = properties
> +
> +class BaseDevice:
> + """A base class to represent device data from HAL and udev."""

There should be a blank line here? (like all the other classes in this file).

> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-01 07:42:22 +0000
> @@ -380,6 +380,51 @@
> 'Unexpected value of HALDevice.parent_udi, '
> 'when no parent information available.')
>
> + def testHALDeviceDeviceId(self):

I'm guessing you're aware that these should be pep8-style names,

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

but in this case the rest of the file uses camel-case, so for consistency...

> + """Test of HALDevice.device_id."""
> + properties = {}
> + parser = SubmissionParser(self.log)
> + device = HALDevice(1, '/some/udi/path', properties, parser)
> + self.assertEqual(
> + '/some/udi/path', device.device_id,
> + 'Unexpected value of HALDevice.parent_udi')

Should this be 'Unexpected value of HALDevice.device_id (which actually
returns self.udi not parent_udi?)

> +
> + def testHALDevicePciClass(self):
> + """Test of HALDevice.pci_class."""
> + properties = {
> + 'pci.device_class': (1, 'int'),
> + }
> + parser = SubmissionParser(self.log)
> + device = HALDevice(1, '/some/udi/path', properties, parser)
> + ...

Read more...

review: Approve (code)
Revision history for this message
Abel Deuring (adeuring) wrote :
Download full text (5.2 KiB)

Hi Michael,

thanks for the review!

On 01.10.2009 10:15, Michael Nelson wrote:
> Review: Approve code
>> This branch is a first step to refactor class HALDevice in
>> l/c/l/scripts/hwdbsubmissions.py
>>
> ...
>
> r=me, just a few small copy-n-paste/ordering issues below.
>
> Thanks for the detailed description! Makes much more sense now :) It's
> nice seeing the preparation for the new UDevDevice in a separate small
> branch!
>
>
>> === modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
>> --- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-09-29 15:05:09 +0000
>> +++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-01 07:42:22 +0000
>> @@ -7,7 +7,7 @@
>> data and for the community test submissions.
>> """
>>
>> -
>> +__metaclass__ = type
>> __all__ = [
>> 'SubmissionParser',
>> 'process_pending_submissions',
>> @@ -1235,59 +1235,29 @@
>> root_device.createDBData(submission, None)
>> return True
>>
>> -class HALDevice:
>> - """The representation of a HAL device node."""
>> -
>> - def __init__(self, id, udi, properties, parser):
>> - """HALDevice constructor.
>> -
>> - :param id: The ID of the HAL device in the submission data as
>> - specified in <device id=...>.
>> - :type id: int
>> - :param udi: The UDI of the HAL device.
>> - :type udi: string
>> - :param properties: The HAL properties of the device.
>> - :type properties: dict
>> - :param parser: The parser processing a submission.
>> - :type parser: SubmissionParser
>> - """
>> - self.id = id
>> - self.udi = udi
>> - self.properties = properties
>> +
>> +class BaseDevice:
>> + """A base class to represent device data from HAL and udev."""
>
> There should be a blank line here? (like all the other classes in this file).

Fixed.

>
>> === modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
>> --- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-06-25 05:30:52 +0000
>> +++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-01 07:42:22 +0000
>> @@ -380,6 +380,51 @@
>> 'Unexpected value of HALDevice.parent_udi, '
>> 'when no parent information available.')
>>
>> + def testHALDeviceDeviceId(self):
>
> I'm guessing you're aware that these should be pep8-style names,
>
> https://dev.launchpad.net/TestsStyleGuide#Python Test Cases
>
> but in this case the rest of the file uses camel-case, so for consistency...
>
>> + """Test of HALDevice.device_id."""
>> + properties = {}
>> + parser = SubmissionParser(self.log)
>> + device = HALDevice(1, '/some/udi/path', properties, parser)
>> + self.assertEqual(
>> + '/some/udi/path', device.device_id,
>> + 'Unexpected value of HALDevice.parent_udi')
>
> Should this be 'Unexpected value of HALDevice.device_id (which actually
> returns self.udi not parent_udi?)

oops, sure. Fixed.

>
>> +
>> + def testHALDevicePciClass(self):
>> + """Test of...

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/scripts/hwdbsubmissions.py'
2--- lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-09-29 15:05:09 +0000
3+++ lib/canonical/launchpad/scripts/hwdbsubmissions.py 2009-10-01 09:10:27 +0000
4@@ -7,7 +7,7 @@
5 data and for the community test submissions.
6 """
7
8-
9+__metaclass__ = type
10 __all__ = [
11 'SubmissionParser',
12 'process_pending_submissions',
13@@ -1235,59 +1235,30 @@
14 root_device.createDBData(submission, None)
15 return True
16
17-class HALDevice:
18- """The representation of a HAL device node."""
19-
20- def __init__(self, id, udi, properties, parser):
21- """HALDevice constructor.
22-
23- :param id: The ID of the HAL device in the submission data as
24- specified in <device id=...>.
25- :type id: int
26- :param udi: The UDI of the HAL device.
27- :type udi: string
28- :param properties: The HAL properties of the device.
29- :type properties: dict
30- :param parser: The parser processing a submission.
31- :type parser: SubmissionParser
32- """
33- self.id = id
34- self.udi = udi
35- self.properties = properties
36+
37+class BaseDevice:
38+ """A base class to represent device data from HAL and udev."""
39+
40+ def __init__(self, parser):
41 self.children = []
42 self.parser = parser
43 self.parent = None
44
45+ # Translation of the HAL info.bus/info.subsystem property and the
46+ # udev property SUBSYSTEM to HWBus enumerated buses.
47+ subsystem_hwbus = {
48+ 'pcmcia': HWBus.PCMCIA,
49+ 'usb_device': HWBus.USB,
50+ 'ide': HWBus.IDE,
51+ 'serio': HWBus.SERIAL,
52+ }
53+
54 def addChild(self, child):
55 """Add a child device and set the child's parent."""
56 assert type(child) == type(self)
57 self.children.append(child)
58 child.parent = self
59
60- def getProperty(self, property_name):
61- """Return the property property_name.
62-
63- Note that there is no check of the property type.
64- """
65- if property_name not in self.properties:
66- return None
67- name, type_ = self.properties[property_name]
68- return name
69-
70- @property
71- def parent_udi(self):
72- """The UDI of the parent device."""
73- return self.getProperty('info.parent')
74-
75- # Translation of the HAL info.bus/info.subsystem property to HWBus
76- # enumerated buses.
77- hal_bus_hwbus = {
78- 'pcmcia': HWBus.PCMCIA,
79- 'usb_device': HWBus.USB,
80- 'ide': HWBus.IDE,
81- 'serio': HWBus.SERIAL,
82- }
83-
84 # Translation of subclasses of the PCI class storage to HWBus
85 # enumerated buses. The Linux kernel accesses IDE and SATA disks
86 # and CDROM drives via the SCSI system; we want to know the real bus
87@@ -1307,6 +1278,22 @@
88 7: HWBus.SAS,
89 }
90
91+ @property
92+ def device_id(self):
93+ """A unique ID for this device."""
94+ raise NotImplementedError()
95+
96+ @property
97+ def pci_class(self):
98+ """The PCI device class of the device or None for Non-PCI devices."""
99+ raise NotImplementedError()
100+
101+ @property
102+ def pci_subclass(self):
103+ """The PCI device sub-class of the device or None for Non-PCI devices.
104+ """
105+ raise NotImplementedError()
106+
107 def translateScsiBus(self):
108 """Return the real bus of a device where raw_bus=='scsi'.
109
110@@ -1321,29 +1308,29 @@
111 parent = self.parent
112 if parent is None:
113 self.parser._logWarning(
114- 'Found SCSI device without a parent: %s.' % self.udi)
115+ 'Found SCSI device without a parent: %s.' % self.device_id)
116 return None
117 grandparent = parent.parent
118 if grandparent is None:
119 self.parser._logWarning(
120- 'Found SCSI device without a grandparent: %s.' % self.udi)
121+ 'Found SCSI device without a grandparent: %s.'
122+ % self.device_id)
123 return None
124
125 grandparent_bus = grandparent.raw_bus
126 if grandparent_bus == 'pci':
127- if (grandparent.getProperty('pci.device_class')
128- != PCI_CLASS_STORAGE):
129+ if (grandparent.pci_class != PCI_CLASS_STORAGE):
130 # This is not a storage class PCI device? This
131 # indicates a bug somewhere in HAL or in the hwdb
132 # client, or a fake submission.
133- device_class = grandparent.getProperty('pci.device_class')
134+ device_class = grandparent.pci_class
135 self.parser._logWarning(
136 'A (possibly fake) SCSI device %s is connected to '
137 'PCI device %s that has the PCI device class %s; '
138 'expected class 1 (storage).'
139- % (self.udi, grandparent.udi, device_class))
140+ % (self.device_id, grandparent.device_id, device_class))
141 return None
142- pci_subclass = grandparent.getProperty('pci.device_subclass')
143+ pci_subclass = grandparent.pci_subclass
144 return self.pci_storage_subclass_hwbus.get(pci_subclass)
145 elif grandparent_bus == 'usb':
146 # USB storage devices have the following HAL device hierarchy:
147@@ -1395,26 +1382,78 @@
148 # subclass 7).
149 # XXX Abel Deuring 2005-05-14 How can we detect ExpressCards?
150 # I do not have any such card at present...
151- parent_class = self.parent.getProperty('pci.device_class')
152- parent_subclass = self.parent.getProperty('pci.device_subclass')
153+ parent_class = self.parent.pci_class
154+ parent_subclass = self.parent.pci_subclass
155 if (parent_class == PCI_CLASS_BRIDGE
156 and parent_subclass == PCI_SUBCLASS_BRIDGE_CARDBUS):
157 return HWBus.PCCARD
158 else:
159 return HWBus.PCI
160
161- translate_bus_name = {
162- 'pci': translatePciBus,
163- 'scsi': translateScsiBus,
164- }
165-
166- @property
167- def raw_bus(self):
168- """Return the device bus as specified by HAL.
169-
170- Older versions of HAL stored this value in the property
171- info.bus; newer versions store it in info.subsystem.
172- """
173+ @property
174+ def raw_bus(self):
175+ """Return the device bus as specified by HAL or udev."""
176+ raise NotImplementedError()
177+
178+
179+class HALDevice(BaseDevice):
180+ """The representation of a HAL device node."""
181+
182+ def __init__(self, id, udi, properties, parser):
183+ """HALDevice constructor.
184+
185+ :param id: The ID of the HAL device in the submission data as
186+ specified in <device id=...>.
187+ :type id: int
188+ :param udi: The UDI of the HAL device.
189+ :type udi: string
190+ :param properties: The HAL properties of the device.
191+ :type properties: dict
192+ :param parser: The parser processing a submission.
193+ :type parser: SubmissionParser
194+ """
195+ super(HALDevice, self).__init__(parser)
196+ self.id = id
197+ self.udi = udi
198+ self.properties = properties
199+
200+ def getProperty(self, property_name):
201+ """Return the HAL property property_name.
202+
203+ Note that there is no check of the property type.
204+ """
205+ if property_name not in self.properties:
206+ return None
207+ name, type_ = self.properties[property_name]
208+ return name
209+
210+ @property
211+ def parent_udi(self):
212+ """The UDI of the parent device."""
213+ return self.getProperty('info.parent')
214+
215+ @property
216+ def device_id(self):
217+ """See `BaseDevice`."""
218+ return self.udi
219+
220+ @property
221+ def pci_class(self):
222+ """See `BaseDevice`."""
223+ return self.getProperty('pci.device_class')
224+
225+ @property
226+ def pci_subclass(self):
227+ """The PCI device sub-class of the device or None for Non-PCI devices.
228+ """
229+ return self.getProperty('pci.device_subclass')
230+
231+ @property
232+ def raw_bus(self):
233+ """See `BaseDevice`."""
234+ # Older versions of HAL stored this value in the property
235+ # info.bus; newer versions store it in info.subsystem.
236+ #
237 # Note that info.bus is gone for all devices except the
238 # USB bus. For USB devices, the property info.bus returns more
239 # detailed data: info.subsystem has the value 'usb' for all
240@@ -1436,7 +1475,7 @@
241 cannot be determined.
242 """
243 device_bus = self.raw_bus
244- result = self.hal_bus_hwbus.get(device_bus)
245+ result = self.subsystem_hwbus.get(device_bus)
246 if result is not None:
247 return result
248
249@@ -1612,8 +1651,8 @@
250 # possible bridges, like ISA->USB..
251 parent = self.parent
252 parent_bus = parent.raw_bus
253- parent_class = parent.getProperty('pci.device_class')
254- parent_subclass = parent.getProperty('pci.device_subclass')
255+ parent_class = parent.pci_class
256+ parent_subclass = parent.pci_subclass
257 if (parent_bus == 'pci'
258 and parent_class == PCI_CLASS_SERIALBUS_CONTROLLER
259 and parent_subclass == PCI_SUBCLASS_SERIALBUS_USB):
260
261=== modified file 'lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py'
262--- lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-06-25 05:30:52 +0000
263+++ lib/canonical/launchpad/scripts/tests/test_hwdb_submission_processing.py 2009-10-01 09:10:27 +0000
264@@ -380,6 +380,51 @@
265 'Unexpected value of HALDevice.parent_udi, '
266 'when no parent information available.')
267
268+ def testHALDeviceDeviceId(self):
269+ """Test of HALDevice.device_id."""
270+ properties = {}
271+ parser = SubmissionParser(self.log)
272+ device = HALDevice(1, '/some/udi/path', properties, parser)
273+ self.assertEqual(
274+ '/some/udi/path', device.device_id,
275+ 'Unexpected value of HALDevice.device_id')
276+
277+ def testHALDevicePciClass(self):
278+ """Test of HALDevice.pci_class."""
279+ properties = {
280+ 'pci.device_class': (1, 'int'),
281+ }
282+ parser = SubmissionParser(self.log)
283+ device = HALDevice(1, '/some/udi/path', properties, parser)
284+ self.assertEqual(
285+ 1, device.pci_class,
286+ 'Unexpected value of HALDevice.pci_class.')
287+
288+ properties = {}
289+ parser = SubmissionParser(self.log)
290+ device = HALDevice(1, '/some/udi/path', properties, parser)
291+ self.assertEqual(
292+ None, device.pci_class,
293+ 'Unexpected value of HALDevice.pci_class for Non-PCI device.')
294+
295+ def testHALDevicePciSubClass(self):
296+ """Test of HALDevice.pci_subclass."""
297+ properties = {
298+ 'pci.device_subclass': (1, 'int'),
299+ }
300+ parser = SubmissionParser(self.log)
301+ device = HALDevice(1, '/some/udi/path', properties, parser)
302+ self.assertEqual(
303+ device.pci_subclass, 1,
304+ 'Unexpected value of HALDevice.pci_subclass.')
305+
306+ properties = {}
307+ parser = SubmissionParser(self.log)
308+ device = HALDevice(1, '/some/udi/path', properties, parser)
309+ self.assertEqual(
310+ None, device.pci_subclass,
311+ 'Unexpected value of HALDevice.pci_sub_class for Non-PCI device.')
312+
313 def testHalDeviceRawBus(self):
314 """test of HALDevice.raw_bus."""
315 properties = {