new device API#579
Conversation
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
Please document the intended behavior and usage of this API. More or less update module docstring in qubes/devices.py. Besides updating what's currently there, I'm interested especially in two more things:
- differences between "assign" and "attach" and how they interact with starting a qube and connecting a device
- device identification: which properties are used to identify the device when
As for more specific comments:
- you use vendor/product as strings from hwdata - are they stored anywhere (or otherwise expected to not change)? in rare cases they may change, for example when somebody fixes a typo in hwdata, or add info about the device that wasn't included there before
- I see new
device-addedevent, but nodevice-removed(or similar), at least in the documentation (but block extension actually sendsdevice-removedevent) - why only PCI devices can be "required"?
|
Ok, I'm writing the docs
The aim was that the device identification is "human-readable" and even more "non-tech-user-readable", meaning in
So I need to add this to documentation (there are
I think we've decided that there is no benefit in making USB/block |
What if I want a qube to which my USB Bluetooth controller is permanently attached?
The USB bus number changing is itself a problem, IMO. |
I'm not sure if we can ensure static bus numbers, but PCI devices should not be reordered anymore. So, maybe you can replace bus number in the ID with PCI device providing it (see symlink target of As for "required" - for USB indeed it's a tricky problem, but I'd prefer this limitation to be enforced at the frontend side, or in extensions for specific device classes, not in the generic code in
It's good to show human readable name (translated via hwdata) to humans, but internally IMO it should use numeric vendor/product. |
You can have USB Bluetooth controller auto-attached to a qube. |
marmarek
left a comment
marmarek
left a comment
There was a problem hiding this comment.
few comments on the recent push; I'm waiting with the full review for the documentation.
| self_id = self._interface_num | ||
| else: | ||
| # partition number: 'sda1' -> '1' | ||
| self_id = self.ident[3:] |
There was a problem hiding this comment.
That wont always work, there can be other names (xvdi1), or even different schemes (nvme0n1p2). Generally, to get partition number, take last number (not just last digit, there can be sda12). But there is still one corner case - whole device ending with a number, like nvme0n1, or loop0 - in those cases the partition number is separated with the p, but you can't tell it's a partition just because it ends with a number... Does the partition device always has "whole block device" as a parent? If so, you can use that fact and only then take partition number. But if parent device is not a block device, then it isn't a partition, so getting just the final number is not appropriate.
There was a problem hiding this comment.
Youre right, I fixed it
| @qubes.ext.handler('domain-start') | ||
| async def on_domain_start(self, vm, _event, **_kwargs): | ||
| # pylint: disable=unused-argument | ||
| for assignment in vm.devices['block'].get_assigned_devices(): | ||
| asyncio.ensure_future(self._attach_and_notify( | ||
| vm, assignment.device, assignment.options)) |
There was a problem hiding this comment.
Block devices attached at domain start can be specified in libvirt xml, in fact you already adjust that part.
There was a problem hiding this comment.
Block devices that are assigned when VM is starting are attached already at https://github.com/QubesOS/qubes-core-admin/pull/579/files#diff-bd38d7fc85084f576ddb6d1d4702f72158e0e30d5e1deb15cf3d49c28c0eb82eR152-R156. So, it looks like you attach them again at domain-start event here.
There was a problem hiding this comment.
Block devices attached at domain startup should be specified in libvirt xml (linked above), instead of attached after domain startup. This is especially important for "required" devices, as one may need them during system startup (for example some standalone qube may have it listed in /etc/fstab and startup will fail without it).
DemiMarie
left a comment
DemiMarie
left a comment
There was a problem hiding this comment.
Would this be a good opportunity to use diskseq to identify block devices? diskseq + partition number is guaranteed to be unique until the backend is rebooted, whereas other identifiers are not guaranteed to be unique. Right now this rule is violated by loop and device-mapper devices, but that’s a kernel bug.
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
0856651 to
24bfe65
Compare
Not really, here we extract device info basing on qubes-db data, so it's just string processing. |
Most parts of docstring in |
piotrbartman
force-pushed
the
prb/new_device_api
branch
2 times, most recently
from
04db171 to
842ed20
Compare
Oh, I missed you already updated it. |
There was a problem hiding this comment.
Since the API now considers also self_identity when automatically attaching devices (which is good!), I think there need to be support for multiple assignments of the same port but with different device self_identity. For example, I may have 3 different devices I'd like to automatically connect to a qube X (when plugged into specific port). Ofc only one of them can be attached at the same time, but I think it should still be possible to have assignments there.
This is problematic for the API, because some places use backend+ident as unique identifier for the assignment. I made a comment about it in one place, but the problem is in several more places.
Maybe in such cases there can be some suffix to the device ident, like yet another +something (hash of self_identity if not self_identity itself? Or not only in such cases, but always? And then for port assignments that would be +any.
There can be a corner case of multiple assignments of the same port, all with required=True. That of course will always fail to start, I think such configuration should still be allowed (for example when user adds new device just to remove the old one later). Maybe a startup message can include a message when such configuration will be detected.
BTW, one day maybe we will have a bus that has unspoofable device identifiers (based on some cryptography?). When that happens, maybe it will make sense to create assignment that has only backend and self_identity (or some other property then?) but has any in place of the current ident. Or maybe, for such trusted identities use it instead of the device port (not sure if a good idea...)? In any case, it would allow automatically attaching specific device regardless of which port it's plugged in. Such support may require some more changes (for example (potential) assignment would not have a specific port value, but it will need to be resolved when actually attaching the device), so lets leave implementation for another iteration. But it's good to think about such possibility when designing API interface.
| !include-service admin.vm.device.mic.Detach * include/admin-local-rwx | ||
| !include-service admin.vm.device.mic.List * include/admin-local-ro | ||
| !include-service admin.vm.device.mic.Set.persistent * include/admin-local-rwx | ||
| !include-service admin.vm.device.mic.Set.assignment * include/admin-local-rwx |
There was a problem hiding this comment.
This is supposed to be about specific DeviceAssignment property, so I guess there should be policy about required and attach_automatically.
| device_assignments, devclass=devclass) | ||
|
|
||
| dev_info = { | ||
| f'{assignment.backend_domain}+{assignment.ident}': |
There was a problem hiding this comment.
This is problematic to use dict with just backend+ident here - there may be multiple assignments for different devices plugged into the same port (they will differ in self_identity, but will have the same ident)
See more about it in the generic comment.
| Update assignment of already attached device. | ||
|
|
||
| Payload: | ||
| `None` -> unassign device from qube |
There was a problem hiding this comment.
This is the same as the Unassign method, no?
| `False` -> device will be auto-attached to qube | ||
| `True` -> device is required to start qube |
There was a problem hiding this comment.
So, those in fact set the required flag, so the method should be called admin.vm.device.{endpoint}.Set.required.
There was a problem hiding this comment.
Not only, if "required" is None it modify attach_automatically flag, so whole assignment. In fact the assignment has only 3 valid states, and setting attach_automatically and required separably can lead to non-valid state. In other words, DeviceAssignment has one state, but exposed to the user/programmer as 2 flags. I even consider to put DeviceAssignment.__required and DeviceAssignment.__automatically_attach as one variable with 3 possible values.
There was a problem hiding this comment.
if "required" is
Noneit modifyattach_automaticallyflag
But this is "Unassign" method listed above, no?
There was a problem hiding this comment.
ok, now I get it, I will correct that
| keys = [] | ||
| values = [] | ||
| key, _, rest = rest.partition("='") | ||
| keys.append(key) | ||
| while "='" in rest: | ||
| value_key, _, rest = rest.partition("='") | ||
| value, _, key = value_key.rpartition("' ") | ||
| values.append(deserialize_str(value)) | ||
| keys.append(key) | ||
| value = rest[:-1] # ending ' | ||
| values.append(deserialize_str(value)) | ||
|
|
||
| properties = dict() | ||
| for key, value in zip(keys, values): | ||
| if key.startswith("_"): | ||
| # it's handled in cls.__init__ | ||
| properties[key[1:]] = value | ||
| else: | ||
| properties[key] = value |
There was a problem hiding this comment.
Move this to a helper function? It's used in at least two places.
| @qubes.ext.handler('domain-start') | ||
| async def on_domain_start(self, vm, _event, **_kwargs): | ||
| # pylint: disable=unused-argument | ||
| for assignment in vm.devices['block'].get_assigned_devices(): | ||
| asyncio.ensure_future(self._attach_and_notify( | ||
| vm, assignment.device, assignment.options)) |
| super().__init__( | ||
| backend_domain=backend_domain, ident=ident, devclass="pci") | ||
|
|
||
| if hasattr(self, 'regex'): |
There was a problem hiding this comment.
This if was necessary when it was in a generic class. Here, the regex attribute is always set.
| @property | ||
| def product(self) -> str: | ||
| """ | ||
| Device name from local database `/usr/share/hwdata/usb.ids` |
There was a problem hiding this comment.
| Device name from local database `/usr/share/hwdata/usb.ids` | |
| Device name from local database `/usr/share/hwdata/pci.ids` |
Agree, but as I mentioned in the last section 'To Do and to Consider' in the PR description, I believe this change belongs to the next iteration, as it requires modifications in several places, and I think currently there are already quite a few changes done. |
| @property | ||
| def attachment(self) -> Optional['qubes.vm.BaseVM']: | ||
| """ | ||
| Warning: this property is time-consuming, do not run in loop! | ||
| """ | ||
| if not self.backend_domain.is_running(): | ||
| return None | ||
| for vm in self.backend_domain.app.domains: | ||
| if not vm.is_running(): | ||
| continue | ||
| if self._is_attached_to(vm): | ||
| return vm | ||
|
|
||
| return None |
There was a problem hiding this comment.
Should there be an index to make looking this up fast?
There was a problem hiding this comment.
Probably, but we need to remember that it should always be up to date (not cached), if you have an idea how to do it, I'm in.
There was a problem hiding this comment.
Updating it whenever a device is attached or detached is the first idea that comes to mind. Caches are fine so long as they are invalidated synchronously when needed.
There was a problem hiding this comment.
BlockDeviceExtension already has a devices_cache, but attachment is intended to return the actual state (including, in particular, situations where something went wrong and the potential cache is outdated). For API compatibility, this is a property, and the warning is there because when we want to get information about attachment for a bunch of devices, we should use some form of bulk data loading (just like we do for updating devices_cache) rather than in a loop.
There was a problem hiding this comment.
Are there any situations where the cache could be outdated, or is that always a bug?
There was a problem hiding this comment.
Not sure, but if it is predictable it's already update a cache. Maybe if you directly use libvirt to attach device it will bypass mechanism, but attachment still will work.
|
@marmarek Can you elaborate? |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024061301-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024052808-4.3&flavor=update
Failed tests23 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/101100#dependencies 38 fixed
Unstable testsDetails
|
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
33e111b to
58ce9ee
Compare
|
Integration tests need an update: https://openqa.qubes-os.org/tests/96815 And unit tests detected I believe a real issue - meminfo-writer service (as well as enabling/disabling dynamic memory management) is toggled based on PCI devices presence - this part likely misbehave now. |
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
58ce9ee to
dae6da2
Compare
there is no good reason to allow a space in the option's key
The unassignment of a required device does not break anything, and it's required to detach the required device. Detaching required device can break things but should be possible.
update pci description format
usb device proxy needs this feature
piotrbartman
force-pushed
the
prb/new_device_api
branch
from
74e7de1 to
58b7872
Compare
4 participants
need QubesOS/qubes-linux-utils/pull/111
draft of implementation of new device API.
split persistent flag into automatically_attach and required, so usb devices can be auto-attached but in case of lack of device or renumbering usb bus we can still start a domain.
TODO: move Device DeviceInterface, DeviceCategorym DeviceInfo, DeviceAssignment etc. to one place (it is shared between this package and client)
TODO: tests in this repo can be broken
To Do and to Considerate
Currently, we can make one assignment of a selected port to a single machine in two ways:
identity='any', which means that anything plugged into the designated port will be automatically attached to the VM, as it has been functioning thus far.An open issue is what to do with a device that attempts to forcibly present itself in various ways. At the moment, I consider such an attack to be unlikely.
Another consideration is the user interface. It would be beneficial to provide options such as:
The handling of USB bus number changes is still not implemented. According to @marmarek's suggestion, we could check the parent device (PCI) in this regard. However, in the current configuration, this would require significant refactoring, as the current protocol relies on device identification as
backend-vm:bus-port. Thus, we need to find a method to ensure that this number is always correct. This will also address issues with block devices because currently, if we plug in USB sticks in different orders or do not properly eject and reinsert one of them, theidentof mass storage devices changes, and automatic attachment fails. Theoretically, usingself_identityresolves this issue, but there are still assumptions in many places thatbackend-vm:block-nameidentifies the device, which is not straightforward.QubesOS/qubes-issues#4626