udevadm parser picking wrong identifiers for devices
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Checkbox |
Fix Released
|
High
|
Daniel Manrique |
Bug Description
In the course of debugging bug 1211369 and bug 1210485, I added some more thorough testing to the checkbox udev parser.
https:/
This hasn't been proposed for merge yet, because the point of this bug is that the parser is picking incorrect data for some devices. For instance, the udev data from the Dell Vostro V131 says it has a wired and a wireless device (it has two wireless devices, I'm ignoring the wimax device for now, focusing on the core bug):
E: ID_MODEL_
E: ID_VENDOR_
E: PCI_ID=10EC:8168
However, the parser returns the PCI IDs from the *bus*, which is an Intel device:
[<UdevadmDevice: bus: pci id [8086:1c12] RTL8111/8168 PCI Express Gigabit Ethernet controller>]
Something similar happens with the wireless Atheros device:
E: ID_MODEL_
E: ID_VENDOR_
E: PCI_ID=168C:002B
<UdevadmDevice: bus: pci id [8086:1c16] AR9285 Wireless Network Adapter (PCI-Express)>
Also, for some systems it's unable to find the PCI IDs at all, on a Lenovo T420 and T430 it can't get the IDs for the wired network device.
The branch linked to at the beginning of the comment has the data files from the real systems, cd checkbox-old and run:
PYTHONPATH=. ./setup.py test --test-suite checkbox.
this will run the test suite for the parser and show the errors.
Related branches
- Daniel Manrique (community): Needs Resubmitting
- Zygmunt Krynicki (community): Approve
-
Diff: 5435 lines (+5148/-38)5 files modifiedcheckbox-old/checkbox/parsers/tests/test_submission.py (+2/-2)
checkbox-old/checkbox/parsers/tests/test_udevadm.py (+80/-19)
checkbox-old/checkbox/parsers/tests/udevadm_data/LENOVO_T420.txt (+5034/-0)
checkbox-old/checkbox/parsers/udevadm.py (+30/-17)
checkbox-old/debian/changelog (+2/-0)
Changed in checkbox: | |
status: | In Progress → Fix Committed |
Changed in checkbox: | |
status: | Fix Committed → Fix Released |
OK, I seem to have found the guilty commit (man I could have done a bisect for this). Anyway, it's this one:
Revision:
2206.2.28
13-07-08 11:47 AM
checkbox- old:parsers: udevadm: Clean the network devices path
To stay compatible with checkbox-old local jobs using NETWORK path to ------- -------
find the registered interface.
-------
If I revert that commit, all the tests (save for one which I'm still looking into) pass, so at least for the systems I declared per-device checks for, I'm getting the correct IDs and data. For instance, for the T420 which is our "control", I now have:
{
"bus" : "pci", #Correct PCI bus, yay!
"category" : "WIRELESS",
"driver" : "iwlwifi",
"path" : "/devices/ pci0000: 00/0000: 00:1c.1/ 0000:03: 00.0/net/ wlan0", #Unclean path ;( need to find a way to fix this
"product" : "Centrino Advanced-N 6205 [Taylor Peak]",
"product_ id": 133, #Note this is the correct product_id per the PCI device database
"subproduct_ id": null,
"subvendor_ id": null,
"vendor" : "Intel Corporation",
"vendor_ id": 32902 #Also correct, it's 0x8086
},
This other device also looks good:
{
"bus" : "pci",
"category" : "NETWORK",
"driver" : "e1000e",
"path" : "/devices/ pci0000: 00/0000: 00:19.0/ net/eth0" ,
"product" : "82579LM Gigabit Network Connection",
"product_ id": 5378,
"subproduct_ id": null,
"subvendor_ id": null,
"vendor" : "Intel Corporation",
"vendor_ id": 32902
},
Compare this with borked data with the faulty commit in place:
{
"bus" : "pci",
"category" : "WIRELESS",
"driver" : "iwlwifi",
"path" : "/devices/ pci0000: 00/0000: 00:1c.1/ 0000:03: 00.0", #The path looks better in theory
"product" : "Centrino Advanced-N 6205 [Taylor Peak]",
"product_ id": 7186, #Wrong product ID, matches the PCI bridge
"subproduct_ id": null,
"subvendor_ id": null,
"vendor" : "Intel Corporation",
"vendor_ id": 32902
},
And this:
"bus" : "net", #Wrong bus, we want pci
"category" : "NETWORK",
"driver" : "e1000e",
"path" : "/devices/ pci0000: 00/0000: 00:19.0" , #Path also looks nicer but ...
"product" : "82579LM Gigabit Network Connection",
"product_ id": null, #This is bad :(
"subproduct_ id": null,
"subvendor_ id": null,
"vendor" : "Intel Corporation",
"vendor_ id": null #This is also bad.
{
},
OK, my next step is to fix the one test problem with the removed commit, then go back and try to replicate the path cleaning behavior while keeping the rest of the correct data (or understanding the rationale behind that commit and deciding if we can revert it outright).