Skip to content

Return UnknownDevice for assignments where device is unknown #400

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.

Sign up for GitHub

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

Open
marmarta wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Return UnknownDevice for assignments where device is unknown #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion qubesadmin/device_protocol.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1367,7 +1367,9 @@ def device(self) -> DeviceInfo:
return devices[0]
if len(devices) > 1:
raise ProtocolError("Too many devices matches to assignment")
raise ProtocolError("No devices matches to assignment")
return UnknownDevice(port=self.port,
device_id=self.device_id,
attachment=self)
Copy link
Member

@marmarek marmarek Dec 10, 2025

Choose a reason for hiding this comment

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

First, check carefully places where this attribute is used (in global config, VM settings, widgets, ansible etc). The behavior change may be unexpected in some, especially since the old behavior was explicitly documented in the docstring.

Then, update docstring.

And finally, I don't think attachment attribute should be set. Its docstring says "VM to which device is attached (frontend domain)", which cannot be true for not present device.

Copy link
Member Author

@marmarta marmarta Feb 10, 2026

Choose a reason for hiding this comment

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

I think you're implying this is a bad idea? I think it makes sense not to raise a ProtocolError if we have an UnknownDevice for those purposes, but I can try to think about tacking this problem in another way.

(and about the attachment attribute, it is incorrect to set here self, yes, but this is a property of a DeviceAssignment that knows its frontend domain, shouldn't we set it?)

Copy link
Member

@marmarek marmarek Feb 10, 2026

Choose a reason for hiding this comment

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

I think the change makes sense in general. But since it's behavior change, it needs at least checking if other places don't rely on the old behavior. For example attach_device in qubesadmin/tools/qvm_device.py seems to rely on this exception. I'm sure there are other places too (maybe also in other repositories...)

(and about the attachment attribute, it is incorrect to set here self, yes, but this is a property of a DeviceAssignment that knows its frontend domain, shouldn't we set it?)

"attachment" is about where the device is attached (like, currently connected), not assigned (potentially attached, automatically or not, may be even multiple assignments for the same device)

Copy link
Member

@marmarek marmarek Feb 10, 2026

Choose a reason for hiding this comment

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

Oh, and note this file is also copied to core-admin, so check if that side doesn't rely on this exception too.


@property
def port(self) -> Port:
Expand Down
21 changes: 12 additions & 9 deletions qubesadmin/tests/mock_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,6 +691,7 @@ def __init__(
vendor: str,
attached: Optional[str] = None,
assigned: Optional[List[Tuple[str, str, list[Any] | None]]] = None,
device_unknown: bool = False,
):
"""
:param qapp: QubesTest object
Expand All @@ -712,6 +713,7 @@ def __init__(
self.assigned = assigned
self.product = product
self.vendor = vendor
self.device_unknown = device_unknown

self.interface = self.device_id.split(":")[-1]

Expand Down Expand Up @@ -761,16 +763,17 @@ def update_calls(self):
)
]

self.qapp.expected_calls[
(
self.backend_vm,
f"admin.vm.device.{self.dev_class}.Available",
None,
None,
if not self.device_unknown:
self.qapp.expected_calls[
(
self.backend_vm,
f"admin.vm.device.{self.dev_class}.Available",
None,
None,
)
] = (
current_response + self.device_string().encode()
)
] = (
current_response + self.device_string().encode()
)

if self.attached:
current_response = self.qapp.expected_calls[
Expand Down