Skip to content

Return UnknownDevice for assignments where device is unknown#400

Open
marmarta wants to merge 1 commit intoQubesOS:mainfrom
marmarta:bugfix10409
Open

Return UnknownDevice for assignments where device is unknown#400
marmarta wants to merge 1 commit intoQubesOS:mainfrom
marmarta:bugfix10409

Conversation

@marmarta
Copy link
Member

@marmarta marmarta commented Dec 8, 2025

@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.10%. Comparing base (a5ea121) to head (33cc033).

Files with missing lines Patch % Lines
qubesadmin/device_protocol.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #400   +/-   ##
=======================================
  Coverage   76.10%   76.10%           
=======================================
  Files          53       53           
  Lines        9287     9287           
=======================================
  Hits         7068     7068           
  Misses       2219     2219           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

marmarta added a commit to marmarta/qubes-manager that referenced this pull request Dec 9, 2025
Fix tests so that now they will fail if there is a bad reaction
to an unknown PCI device. Clean-up handling unknown PCI
devices by settigs.

requires QubesOS/qubes-core-admin-client#400
fixes QubesOS/qubes-issues#10409
raise ProtocolError("No devices matches to assignment")
return UnknownDevice(port=self.port,
device_id=self.device_id,
attachment=self)
Copy link
Member

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

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants