Partial fix for https://github.com/QubesOS/qubes-issues/issues/6587 #559
Conversation
Codecov Report
@@ Coverage Diff @@
## main #559 +/- ##
=======================================
Coverage 67.14% 67.14%
=======================================
Files 55 55
Lines 11131 11128 -3
=======================================
- Hits 7474 7472 -2
+ Misses 3657 3656 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
qubes/devices.py
Outdated
| self._dict = {} | ||
| self._dict = OrderedDict() |
There was a problem hiding this comment.
Regular dicts are already ordered these days:
"Changed in version 3.7: Dictionary order is guaranteed to be insertion order. This behavior was an implementation detail of CPython from 3.6."
There was a problem hiding this comment.
I think it is OK to exclude this change. I've found three reasons to use OrderedDict even in modern Python versions:
- You need some extra method provided by OrderedDict that is missing in dict. It doesn't apply to this PR.
- Equality (===). It doesn't apply to this PR.
- Documentation. By using OrderedDict, you make it explicit that you care about the order. I neither strongly support nor strongly oppose this.
There was a problem hiding this comment.
I dropped OrderedDict then. And also squashed the two commits, so it's easier to follow the history (no more splitting+merging again the function)
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2023101004-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2023071104-4.2&flavor=update
Failed tests35 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/77326#dependencies 23 fixed
Unstable tests
|
See QubesOS/qubes-issues#6587. Attempt to fix the rest of PCI device reordering
marmarek
force-pushed
the
devices-order
branch
from
43aa91b
to
7b8e09b
Compare
| @@ -351,24 +353,19 @@ def assignments(self, persistent=None): | |||
| self._bus)) | |||
| if persistent is True: | |||
| # don't break app.save() | |||
| return self._set | |||
| return list(self._set) | |||
There was a problem hiding this comment.
I know the line looks a bit silly, because it looks like we are converting a set (i.e., randomly sorted collection) to a list. However, self._set is a PersistentCollection, which is backed by dict. So, IIUC, PersistentCollection keeps the order of the items.
Yeah, we could rename self._set to some more descriptive. But it is a set (it cannot contain any item twice), just not an ordinary Python set.
| DeviceAssignment( | ||
| backend_domain=dev.backend_domain, | ||
| ident=dev.ident, options=options, | ||
| bus=self._bus)) | ||
| return result |
There was a problem hiding this comment.
Should this be sorted(result)?
There was a problem hiding this comment.
No. Otherwise adding new device could move another one attached earlier.
This is forward ported version of #407
Original description by @v6ak :
Partial fix for QubesOS/qubes-issues#6587