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

Revision history for this message
Abel Deuring (adeuring) wrote :

This branch fixes a stupid mistake I made in the property UdevDevice.scsi_controller. This property is supposed to return the node of the controller of a SCSI device.

(Background: We get data about hardware mostly from udev; a udev device is represented in the HWDB submission processing script by class UdevDevice. udev devices have parent-child relationships, where device A is an ancestor of device B, if the pathname of device B extends the pathname of device A.)

This mistake happened for two reasons:

1. I copy&pasted the sample data used in tests from "udevadm info --export-db" to the test data as defined in l/c/l/scripts/tests/test_hwdb_submission_processing.py, without looking at the data suffciently carefully. So, when reading and copying a relevant part of this data, like:

P: /devices/pci0000:00/0000:00:1f.1/host4
E: DEVTYPE=scsi_host
E: SUBSYSTEM=scsi

P: /devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4
E: SUBSYSTEM=scsi_host

P: /devices/pci0000:00/0000:00:1f.1/host4/target4:0:0
E: DEVTYPE=scsi_target
E: SUBSYSTEM=scsi

P: /devices/pci0000:00/0000:00:1f.1/host4/target4:0:0/4:0:0:0
E: DEVTYPE=scsi_device
E: DRIVER=sr
E: MODALIAS=scsi:t-0x05
E: SUBSYSTEM=scsi

I managed to ignore the fact that the path names /devices/pci0000:00/0000:00:1f.1/host4/scsi_host/host4 and /devices/pci0000:00/0000:00:1f.1/host4/target4:0:0 say that these nodes are siblings, but simply assumed that the former is the parent of the latter.

2. The method UdevDevice.setUo() defines lots of test data. A single udev device is represented by a dictionary like self.pci_device_data; a set of devices needed for some tests was represented by a sequence like self.scsi_device_hierarchy_data. The method UdevDevice.buildUdevDeviceHierarchy() takes such a list and simply assumed that device N of the sequence is the parent of device N+1.

This way, my wrong assumption about the parent-child relation in the example above is simply mapped into the test data.

So I changed UdevDevice.buildUdevDeviceHierarchy() to build the parent-child relations by looking at the path names of the devices (property device_id). Since the test data is copy&pasted from real-world data, we can be quite sure that the relations defined by this method are now correct.

The method now also returns a dictionary instead of a sequence, allowing to identify "interesting" devices by their path names, instead of a somewhat obscure index.

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)

The complaint about etree is not related to my changes.

« Back to merge proposal