fix block device events

To properly report attachments or detachments of block devices, the logic has been moved to the extension. As a bonus, the following issue is fix: QubesOS/qubes-issues#4692

qubes/ext/block.py

@@ -4,6 +4,7 @@
 #
 # Copyright (C) 2017 Marek Marczykowski-Górecki
 #                               <marmarek@invisiblethingslab.com>
+# Copyright (C) 2024 Piotr Bartman-Szwarc <prbartman@invisiblethingslab.com>
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Lesser General Public
@@ -189,10 +190,41 @@ def parent_device(self) -> Optional[qubes.devices.Device]:
                 self._interface_num = interface_num
         return self._parent
 
+    @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
+
+    def _is_attached_to(self, vm):
+        xml_desc = lxml.etree.fromstring(vm.libvirt_domain.XMLDesc())
+
+        for disk in xml_desc.findall('devices/disk'):
+            info = _try_get_block_device_info(vm.app, disk)
+            if not info:
+                continue
+            backend_domain, ident = info
+
+            if backend_domain.name != self.backend_domain.name:
+                continue
+
+            if self.ident == ident:
+                return True
+        return False
+
     @property
     def self_identity(self) -> str:
         """
-        Get identification of device not related to port.
+        Get identification of a device not related to port.
         """
         parent_identity = ''
         p = self.parent_device
@@ -231,6 +263,32 @@ def _sanitize(
         )
 
 
+def _try_get_block_device_info(app, disk):
+    if disk.get('type') != 'block':
+        return None
+    dev_path_node = disk.find('source')
+    if dev_path_node is None:
+        return None
+
+    backend_domain_node = disk.find('backenddomain')
+    if backend_domain_node is not None:
+        backend_domain = app.domains[
+            backend_domain_node.get('name')]
+    else:
+        backend_domain = app.domains[0]
+
+    dev_path = dev_path_node.get('dev')
+
+    if dev_path.startswith('/dev/'):
+        ident = dev_path[len('/dev/'):]
+    else:
+        ident = dev_path
+
+    ident = ident.replace('/', '_')
+
+    return backend_domain, ident
+
+
 class BlockDeviceExtension(qubes.ext.Extension):
     def __init__(self):
         super().__init__()
@@ -245,28 +303,62 @@ def on_domain_init_load(self, vm, event):
             # avoid building a cache on domain-init, as it isn't fully set yet,
             # and definitely isn't running yet
             current_devices = {
-                dev.ident: dev.attachment
+                dev.ident: dev.attachment  # TODO load all attachments at once
                 for dev in self.on_device_list_block(vm, None)
             }
-            self.devices_cache[vm.name] = current_devices
+            self.devices_cache[vm.name] = current_devices.copy()
         else:
-            self.devices_cache[vm.name] = {}
+            self.devices_cache[vm.name] = {}.copy()
 
     @qubes.ext.handler('domain-qdb-change:/qubes-block-devices')
     def on_qdb_change(self, vm, event, path):
-        """A change in QubesDB means a change in device list."""
+        """A change in QubesDB means a change in a device list."""
         # pylint: disable=unused-argument
-        vm.fire_event('device-list-change:block')
-        current_devices = dict((dev.ident, dev.attachment)
-           for dev in self.on_device_list_block(vm, None))
+        if path is not None:
+            vm.fire_event('device-list-change:block')
+        device_attachments = self.get_device_attachments(vm)
+        current_devices = dict(
+            (dev.ident, device_attachments.get(dev.ident, None))
+            for dev in self.on_device_list_block(vm, None))
+
+        added, attached, detached, removed = (
+            self._compare_cache(vm, current_devices))
 
         # send events about devices detached/attached outside by themselves
-        # (like device pulled out or manual qubes.USB qrexec call)
+        for dev_id, front_vm in detached.items():
+            dev = BlockDevice(vm, dev_id)
+            asyncio.ensure_future(front_vm.fire_event_async(
+                'device-detach:block', device=dev))
+        for dev_id in removed:
+            device = BlockDevice(vm, dev_id)
+            vm.fire_event('device-removed:block', device=device)
+        for dev_id in added:
+            device = BlockDevice(vm, dev_id)
+            vm.fire_event('device-added:block', device=device)
+        for dev_ident, front_vm in attached.items():
+            dev = BlockDevice(vm, dev_ident)
+            asyncio.ensure_future(front_vm.fire_event_async(
+                'device-attach:block', device=dev, options={}))
+
+        self.devices_cache[vm.name] = current_devices.copy()
+
+        for front_vm in vm.app.domains:
+            if not front_vm.is_running():
+                continue
+            for assignment in front_vm.devices['block'].get_assigned_devices():
+                if (assignment.backend_domain == vm
+                        and assignment.ident in added
+                        and assignment.ident not in attached
+                ):
+                    asyncio.ensure_future(self._attach_and_notify(
+                        front_vm, assignment.device, assignment.options))
+
+    def _compare_cache(self, vm, current_devices):
         # compare cached devices and current devices, collect:
         # - newly appeared devices (ident)
         # - devices attached from a vm to frontend vm (ident: frontend_vm)
         # - devices detached from frontend vm (ident: frontend_vm)
-        # - disappeared devices, e.g. plugged out (ident)
+        # - disappeared devices, e.g., plugged out (ident)
         added = set()
         attached = dict()
         detached = dict()
@@ -284,8 +376,8 @@ def on_qdb_change(self, vm, event, path):
                 elif cached_front is None:
                     attached[dev_id] = front_vm
                 else:
-                    # front changed from one to another, so we signal it as:
-                    # detach from first one and attach to the second one.
+                    # a front changed from one to another, so we signal it as:
+                    # detach from the first one and attach to the second one.
                     detached[dev_id] = cached_front
                     attached[dev_id] = front_vm
 
@@ -294,41 +386,33 @@ def on_qdb_change(self, vm, event, path):
                 removed.add(dev_id)
                 if cached_front is not None:
                     detached[dev_id] = cached_front
+        return added, attached, detached, removed
 
-        for dev_id, front_vm in detached.items():
-            dev = BlockDevice(vm, dev_id)
-            asyncio.ensure_future(front_vm.fire_event_async(
-                'device-detach:block', device=dev))
-        for dev_id in removed:
-            device = BlockDevice(vm, dev_id)
-            vm.fire_event('device-removed:block', device=device)
-        for dev_id in added:
-            device = BlockDevice(vm, dev_id)
-            vm.fire_event('device-added:block', device=device)
-        for dev_ident, front_vm in attached.items():
-            dev = BlockDevice(vm, dev_ident)
-            asyncio.ensure_future(front_vm.fire_event_async(
-                'device-attach:block', device=dev, options={}))
+    def get_device_attachments(self, vm_):
+        result = {}
+        for vm in vm_.app.domains:
+            if not vm.is_running():
+                continue
 
-        self.devices_cache[vm.name] = current_devices
+            xml_desc = lxml.etree.fromstring(vm.libvirt_domain.XMLDesc())
 
-        for front_vm in vm.app.domains:
-            if not front_vm.is_running():
-                continue
-            for assignment in front_vm.devices['block'].get_assigned_devices():
-                if (assignment.backend_domain == vm
-                        and assignment.ident in added
-                        and assignment.ident not in attached
-                ):
-                    asyncio.ensure_future(self._attach_and_notify(
-                        front_vm, assignment.device, assignment.options))
+            for disk in xml_desc.findall('devices/disk'):
+                info = _try_get_block_device_info(vm.app, disk)
+                if not info:
+                    continue
+                _backend_domain, ident = info
+
+                result[ident] = vm
+        return result
 
     def device_get(self, vm, ident):
-        '''Read information about device from QubesDB
+        """
+        Read information about a device from QubesDB
 
         :param vm: backend VM object
         :param ident: device identifier
-        :returns BlockDevice'''
+        :returns BlockDevice
+        """
 
         untrusted_qubes_device_attrs = vm.untrusted_qdb.list(
             '/qubes-block-devices/{}/'.format(ident))
@@ -446,7 +530,7 @@ def on_device_pre_attached_block(self, vm, event, device, options):
         if isinstance(device, qubes.devices.UnknownDevice):
             print(f'{device.devclass.capitalize()} device {device} '
                   'not available, skipping.', file=sys.stderr)
-            return
+            raise qubes.devices.UnrecognizedDevice()
 
         # validate options
         for option, value in options.items():
@@ -482,17 +566,34 @@ def on_device_pre_attached_block(self, vm, event, device, options):
                 'This device can be attached only read-only')
 
         if not vm.is_running():
+            print("Not attach, not running", file=sys.stderr)
+            return
+
+        if not isinstance(device, BlockDevice):
+            print("The device is not recognized as block device, "
+                  f"skipping attachment of {device}",
+                  file=sys.stderr)
             return
 
+        if device.attachment:
+            raise qubes.devices.DeviceAlreadyAttached(
+                'Device {!s} already attached to {!s}'.format(
+                    device, device.attachment)
+            )
+
         if not device.backend_domain.is_running():
-            raise qubes.exc.QubesVMNotRunningError(device.backend_domain,
-                'Domain {} needs to be running to attach device from '
-                'it'.format(device.backend_domain.name))
+            raise qubes.exc.QubesVMNotRunningError(
+                device.backend_domain,
+                f'Domain {device.backend_domain.name} needs to be running '
+                f'to attach device from it')
+
+        self.devices_cache[device.backend_domain.name][device.ident] = vm
 
         if 'frontend-dev' not in options:
             options['frontend-dev'] = self.find_unused_frontend(
                 vm, options.get('devtype', 'disk'))
 
+        print(f'attaching {device} exposed by {device.backend_domain.name}')
         vm.libvirt_domain.attachDevice(
             vm.app.env.get_template('libvirt/devices/block.xml').render(
                 device=device, vm=vm, options=options))
@@ -514,6 +615,44 @@ async def _attach_and_notify(self, vm, device, options):
         await vm.fire_event_async(
             'device-attach:block', device=device, options=options)
 
+    @qubes.ext.handler('domain-shutdown')
+    async def on_domain_shutdown(self, vm, event, **_kwargs):
+        """
+        Remove from cache devices attached to or exposed by the vm.
+        """
+        # pylint: disable=unused-argument
+
+        new_cache = {}
+        for domain in vm.app.domains.values():
+            new_cache[domain.name] = {}
+            if domain == vm:
+                for dev_id, front_vm in self.devices_cache[domain.name].items():
+                    if front_vm is None:
+                        continue
+                    dev = BlockDevice(vm, dev_id)
+                    await self._detach_and_notify(vm, dev, options=None)
+                continue
+            for dev_id, front_vm in self.devices_cache[domain.name].items():
+                if front_vm == vm:
+                    dev = BlockDevice(vm, dev_id)
+                    asyncio.ensure_future(front_vm.fire_event_async(
+                            'device-detach:block', device=dev))
+                else:
+                    new_cache[domain.name][dev_id] = front_vm
+        self.devices_cache = new_cache.copy()
+
+    async def _detach_and_notify(self, vm, device, options):
+        # bypass DeviceCollection logic preventing double attach
+        self.on_device_pre_detached_block(
+            vm, 'device-pre-detach:block', device)
+        await vm.fire_event_async(
+            'device-detach:block', device=device, options=options)
+
+    @qubes.ext.handler('qubes-close', system=True)
+    def on_qubes_close(self, app, event):
+        # pylint: disable=unused-argument
+        self.devices_cache.clear()
+
     @qubes.ext.handler('device-pre-detach:block')
     def on_device_pre_detached_block(self, vm, event, device):
         # pylint: disable=unused-argument
@@ -524,6 +663,8 @@ def on_device_pre_detached_block(self, vm, event, device):
         # least)
         for attached_device, options in self.on_device_list_attached(vm, event):
             if attached_device == device:
+                self.devices_cache[device.backend_domain.name][
+                    device.ident] = None
                 vm.libvirt_domain.detachDevice(
                     vm.app.env.get_template('libvirt/devices/block.xml').render(
                         device=device, vm=vm, options=options))

templates/libvirt/xen.xml

@@ -149,11 +149,6 @@
 
             {# start external devices from xvdi #}
             {% set counter = {'i': 4} %}
-            {% for assignment in vm.devices.block.get_assigned_devices(True) %}
-                {% set device = assignment.device %}
-                {% set options = assignment.options %}
-                {% include 'libvirt/devices/block.xml' %}
-            {% endfor %}
 
             {% if vm.netvm %}
                 {% include 'libvirt/devices/net.xml' with context %}