Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial fix for https://github.com/QubesOS/qubes-issues/issues/6587 #559

Merged
merged 3 commits into from
Oct 11, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions qubes/devices.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
`domain-qdb-change:path`) to detect changes and fire
`device-list-change:class` event.
'''
from typing import Optional

import qubes.utils

class DeviceNotAttached(qubes.exc.QubesException, KeyError):
Expand Down Expand Up @@ -331,16 +333,16 @@
'''
return [a.device for a in self._set]

def assignments(self, persistent=None):
def assignments(self, persistent: Optional[bool]=None):
'''List assignments for devices which are (or may be) attached to the
vm.

Devices may be attached persistently (so they are included in
:file:`qubes.xml`) or not. Device can also be in :file:`qubes.xml`,
but be temporarily detached.

:param bool persistent: only include devices which are or are not
attached persistently.
:param Optional[bool] persistent: only include devices which are or are
not attached persistently.
'''

try:
Expand All @@ -351,24 +353,19 @@
self._bus))
if persistent is True:
# don't break app.save()
return self._set
return list(self._set)

Check warning on line 356 in qubes/devices.py

View check run for this annotation

Codecov / codecov/patch

qubes/devices.py#L356

Added line #L356 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could these be sorted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

raise
result = set()
for dev, options in devices:
if dev in self._set and not persistent:
continue
if dev in self._set:
result.add(self._set.get(dev))
elif dev not in self._set and persistent:
continue
else:
result.add(
DeviceAssignment(
backend_domain=dev.backend_domain,
ident=dev.ident, options=options,
bus=self._bus))
if persistent is not False:
result.update(self._set)
result = []
if persistent is not False: # None or True
result.extend(self._set)
if not persistent: # None or False
for dev, options in devices:
if dev not in self._set:
result.append(
DeviceAssignment(
backend_domain=dev.backend_domain,
ident=dev.ident, options=options,
bus=self._bus))
return result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be sorted(result)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Otherwise adding new device could move another one attached earlier.


def available(self):
Expand Down