Skip to content

devices: allow bridge devclass assignment #474

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
fepitre wants to merge 1 commit into
base: main
Choose a base branch
from
Open

devices: allow bridge devclass assignment #474

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
18 changes: 9 additions & 9 deletions qubesadmin/devices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ class is implemented by an extension.
UnknownDevice,
DeviceAssignment,
VirtualDevice,
AssignmentMode, DeviceInterface,
AssignmentMode,
DeviceInterface,
)

if TYPE_CHECKING:
from qubesadmin.vm import QubesVM

Expand Down Expand Up @@ -103,7 +105,7 @@ def assign(self, assignment: DeviceAssignment) -> None:
:param DeviceAssignment assignment: device object
"""
if (
assignment.devclass not in ("pci", "testclass", "block")
assignment.devclass not in self._vm.app.list_deviceclass()

@marmarek marmarek Jun 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't make any sense. It guards which devclasses are allowed as "required", and here you replace it basically with "any". In that case, simply remove this check (and the one below too, as with the change is similarly useless).

But then, ensure at the backend side it's really checked that for example "usb" are not allowed as "required"

@fepitre fepitre Jun 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is there not one that's supposed to exclude tests classes?

@fepitre fepitre Jun 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In all the cases, the spelling is a hell in the new devices API, similar to discussions on Matrix.

@fepitre fepitre Jun 28, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@marmarek are you ok to introduce a flag in device class to notify that it is "required" type class?

@marmarek marmarek Jun 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we have a place to add meta-data of a device class as a whole. But maybe, a device could have a property with list of supported assignment modes? @piotrbartman what do you think?

@fepitre fepitre Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm going into the way of exposing via admin.DeviceClass.List pci+required. It extends the protocol and allows client to know without relying only on back. Especially, it allows to stop hardcoding values. So each device class will have this parameter easily retrieved on call for list of classes.

@marmarek marmarek Jun 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm going into the way of exposing via admin.DeviceClass.List .

That's not a good idea, because it breaks existing clients as they will interpret it whole as a device class (once dom0 is updated, all users, including sys-gui etc will break). This could be solved by adding extra info only if some specific argument is given, like admin.DeviceClass.List+details. But more importantly, it isn't a long term solution, if any other property would be needed, you still don't have a way to send it.

And then, both on the backend and frontend side we need a mechanism to pass this info. On the backend, it could be a class property of relevant DeviceInfo subclass maybe? On the frontend side, some function to return list of classes with extra metadata?

Anyway, I would like to hear @piotrbartman opinion first. Because setting it on individual devices possibly would be more flexible. Or maybe there is some other option I didn't think of.

@piotrbartman piotrbartman Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there not one that's supposed to exclude tests classes?

Only this classes are allowed as "required"

But maybe, a device could have a property with list of supported assignment modes? @piotrbartman what do you think?

We definitely have to keep info from this table somewhere "in the code", available from API. It could be per device.
image

And then, both on the backend and frontend side we need a mechanism to pass this info. On the backend, it could be a class property of relevant DeviceInfo subclass maybe? On the frontend side, some function to return list of classes with extra metadata?

at first glance it seems okay, but I need to think it over

@fepitre fepitre Jun 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The problem is supporting other classes of devices that are not in the codebase => bridges and maybe some other kind too.

and assignment.required
):
raise qubesadmin.exc.QubesValueError(
Expand All @@ -114,8 +116,7 @@ def assign(self, assignment: DeviceAssignment) -> None:
"PCI devices cannot be assigned as not required."
)
if (
assignment.devclass
not in ("testclass", "usb", "block", "mic", "pci")
assignment.devclass not in self._vm.app.list_deviceclass()
and assignment.attach_automatically
):
raise qubesadmin.exc.QubesValueError(
Expand Down Expand Up @@ -271,7 +272,7 @@ def get_exposed_devices(self) -> Iterable[DeviceInfo]:
)

def update_assignment(
self, device: VirtualDevice, required: AssignmentMode
self, device: VirtualDevice, required: AssignmentMode
) -> None:
"""
Update assignment of already attached device.
Expand Down Expand Up @@ -342,8 +343,7 @@ def __missing__(self, key: str) -> DeviceCollection:
def __iter__(self) -> Iterator[str]:
return iter(self._vm.app.list_deviceclass())


def keys(self) -> list[str]: # type: ignore[override]
def keys(self) -> list[str]: # type: ignore[override]
return self._vm.app.list_deviceclass()

def deny(self, *interfaces: Iterable[DeviceInterface]) -> None:
Expand All @@ -354,7 +354,7 @@ def deny(self, *interfaces: Iterable[DeviceInterface]) -> None:
None,
"admin.vm.device.denied.Add",
None,
"".join(repr(ifc) for ifc in interfaces).encode('ascii'),
"".join(repr(ifc) for ifc in interfaces).encode("ascii"),
)

def allow(self, *interfaces: Iterable[DeviceInterface]) -> None:
Expand All @@ -365,7 +365,7 @@ def allow(self, *interfaces: Iterable[DeviceInterface]) -> None:
None,
"admin.vm.device.denied.Remove",
None,
"".join(repr(ifc) for ifc in interfaces).encode('ascii'),
"".join(repr(ifc) for ifc in interfaces).encode("ascii"),
)

def clear_cache(self) -> None:
Expand Down