Merge branch 'devel20200705'

* devel20200705:
  tests: skip gnome-terminal on xfce template flavor
  tests: fix FD leak in qrexec test
  tests: switch default LVM pool to qubes_dom0/vm-pool
  backup: fix error handler for scrypt errors
  Adjust code for possibly coroutine Volume.export() and Volume.export_end()
  storage: add Volume.export_end() function
  backup: add support for calling a function after backing up a file/volume
  backup: call volume.export() just before actually extracting it
  vm/dispvm: place all volumes in the same pool as DispVM's template
  tests: extend TestPool storage driver to make create_on_disk working
  storage: pass a copy of volume_config to pool.init_volume
  tests: cleanup properly in wait_on_fail decorator

doc/qubes-storage.rst

@@ -99,6 +99,10 @@ Methods and properties required to be implemented by the volume class:
  - :py:meth:`~qubes.storage.Volume.export` - return a path to be read to extract
    volume data; for complex formats, this can be a pipe (connected to some
    data-extracting process)
+ - :py:meth:`~qubes.storage.Volume.export_end` - cleanup after exporting the
+   data; this function is called when the path returned by
+   :py:meth:`~qubes.storage.Volume.export` is not used anymore. This method
+   optional - some storage drivers may not implement it if not needed.
  - :py:meth:`~qubes.storage.Volume.import_data` - return a path the data should
    be written to, to import volume data; for complex formats, this can be pipe
    (connected to some data-importing process)

qubes/backup.py

@@ -252,12 +252,30 @@ class Backup:
     # pylint: disable=too-many-instance-attributes
     class FileToBackup:
         # pylint: disable=too-few-public-methods
-        def __init__(self, file_path, subdir=None, name=None, size=None):
+        def __init__(self, file_path_or_func, subdir=None, name=None, size=None,
+                     cleanup_func=None):
+            """Store a single file to backup
+
+            :param file_path_or_func: path to the file or a function
+                returning one; in case of function, it can be a coroutine;
+                if a function is given, *name*, *subdir* and *size* needs to be
+                given too
+            :param subdir: directory in a backup archive to place file in
+            :param name: name of the file in the backup archive
+            :param size: size
+            :param cleanup_func: function to call after processing the file;
+                the function will get the file path as an argument
+            """
+            if callable(file_path_or_func):
+                assert subdir is not None \
+                       and name is not None \
+                       and size is not None
+
             if size is None:
-                size = qubes.storage.file.get_disk_usage(file_path)
+                size = qubes.storage.file.get_disk_usage(file_path_or_func)
 
             if subdir is None:
-                abs_file_path = os.path.abspath(file_path)
+                abs_file_path = os.path.abspath(file_path_or_func)
                 abs_base_dir = os.path.abspath(
                     qubes.config.system_path["qubes_base_dir"]) + '/'
                 abs_file_dir = os.path.dirname(abs_file_path) + '/'
@@ -269,16 +287,19 @@ class Backup:
                 if subdir and not subdir.endswith('/'):
                     subdir += '/'
 
-            #: real path to the file
+            if name is None:
-            self.path = file_path
+                name = os.path.basename(file_path_or_func)
+
+            #: real path to the file (or callable to get one)
+            self.path = file_path_or_func
             #: size of the file
             self.size = size
             #: directory in backup archive where file should be placed
             self.subdir = subdir
             #: use this name in the archive (aka rename)
-            self.name = os.path.basename(file_path)
-            if name is not None:
             self.name = name
+            #: function to call after processing the file
+            self.cleanup_func = cleanup_func
 
     class VMToBackup:
         # pylint: disable=too-few-public-methods
@@ -370,10 +391,11 @@ class Backup:
                 if not volume.save_on_stop:
                     continue
                 vm_files.append(self.FileToBackup(
-                    volume.export(),
+                    volume.export,
                     subdir,
                     name + '.img',
-                    volume.usage))
+                    volume.usage,
+                    cleanup_func=volume.export_end))
 
             vm_files.extend(self.FileToBackup(i, subdir)
                 for i in vm.fire_event('backup-get-files'))
@@ -514,10 +536,9 @@ class Backup:
         if retcode:
             raise qubes.exc.QubesException(
                 "Failed to compute hmac of header file: "
-                + scrypt.stderr.read())
+                + (yield from scrypt.stderr.read()).decode())
         return HEADER_FILENAME, HEADER_FILENAME + ".hmac"
 
-
     def _send_progress_update(self):
         if not self.total_backup_bytes:
             return
@@ -610,18 +631,21 @@ class Backup:
                 # Files will be verified before untaring this.
                 # Prefix the path in archive with filename["subdir"] to have it
                 # verified during untar
+                path = file_info.path
+                if callable(path):
+                    path = yield from qubes.utils.coro_maybe(path())
                 tar_cmdline = (["tar", "-Pc", '--sparse',
-                                '-C', os.path.dirname(file_info.path)] +
+                                '-C', os.path.dirname(path)] +
                                (['--dereference'] if
                                file_info.subdir != "dom0-home/" else []) +
                                ['--xform=s:^%s:%s\\0:' % (
-                                   os.path.basename(file_info.path),
+                                   os.path.basename(path),
                                    file_info.subdir),
-                                   os.path.basename(file_info.path)
+                                   os.path.basename(path)
                                ])
-                file_stat = os.stat(file_info.path)
+                file_stat = os.stat(path)
                 if stat.S_ISBLK(file_stat.st_mode) or \
-                        file_info.name != os.path.basename(file_info.path):
+                        file_info.name != os.path.basename(path):
                     # tar doesn't handle content of block device, use our
                     # writer
                     # also use our tar writer when renaming file
@@ -631,7 +655,7 @@ class Backup:
                         '--override-name=%s' % (
                             os.path.join(file_info.subdir, os.path.basename(
                                 file_info.name))),
-                        file_info.path]
+                        path]
                 if self.compressed:
                     tar_cmdline.insert(-2,
                         "--use-compress-program=%s" % self.compression_filter)
@@ -655,6 +679,10 @@ class Backup:
                     except ProcessLookupError:
                         pass
                     raise
+                finally:
+                    if file_info.cleanup_func is not None:
+                        yield from qubes.utils.coro_maybe(
+                            file_info.cleanup_func(path))
 
                 yield from tar_sparse.wait()
                 if tar_sparse.returncode:

qubes/storage/__init__.py

@@ -203,9 +203,24 @@ class Volume:
             volume data. If extracting volume data require something more
             than just reading from file (for example connecting to some other
             domain, or decompressing the data), the returned path may be a pipe.
+
+            This can be implemented as a coroutine.
+
         '''
         raise self._not_implemented("export")
 
+    def export_end(self, path):
+        """ Cleanup after exporting data.
+
+            This method is called after exporting the volume data (using
+            :py:meth:`export`), when the *path* is not needed anymore.
+
+            This can be implemented as a coroutine.
+
+            :param path: path to cleanup, returned by :py:meth:`export`
+        """
+        # do nothing by default (optional method)
+
     def import_data(self, size):
         ''' Returns a path to overwrite volume data.
 
@@ -423,7 +438,7 @@ class Storage:
         if 'internal' in volume_config:
             # migrate old config
             del volume_config['internal']
-        volume = pool.init_volume(self.vm, volume_config)
+        volume = pool.init_volume(self.vm, volume_config.copy())
         self.vm.volumes[name] = volume
         return volume
 
@@ -634,14 +649,27 @@ class Storage:
                     for target in parsed_xml.xpath(
                         "//domain/devices/disk/target")}
 
+    @asyncio.coroutine
     def export(self, volume):
         ''' Helper function to export volume (pool.export(volume))'''
         assert isinstance(volume, (Volume, str)), \
             "You need to pass a Volume or pool name as str"
-        if isinstance(volume, Volume):
+        if not isinstance(volume, Volume):
-            return volume.export()
+            volume = self.vm.volumes[volume]
+        return (yield from qubes.utils.coro_maybe(volume.export()))
 
-        return self.vm.volumes[volume].export()
+    @asyncio.coroutine
+    def export_end(self, volume, export_path):
+        """ Cleanup after exporting data from the volume
+
+        :param volume: volume that was exported
+        :param export_path: path returned by the export() call
+        """
+        assert isinstance(volume, (Volume, str)), \
+            "You need to pass a Volume or pool name as str"
+        if not isinstance(volume, Volume):
+            volume = self.vm.volumes[volume]
+        yield from qubes.utils.coro_maybe(volume.export_end(export_path))
 
     @asyncio.coroutine
     def import_data(self, volume, size):

qubes/storage/file.py

@@ -21,7 +21,7 @@
 #
 
 ''' This module contains pool implementations backed by file images'''
-
+import asyncio
 import os
 import os.path
 import re
@@ -29,6 +29,7 @@ import subprocess
 from contextlib import suppress
 
 import qubes.storage
+import qubes.utils
 
 BLKSIZE = 512
 
@@ -268,6 +269,7 @@ class FileVolume(qubes.storage.Volume):
         #  if domain is running
         return self.path
 
+    @asyncio.coroutine
     def import_volume(self, src_volume):
         if src_volume.snap_on_start:
             raise qubes.storage.StoragePoolException(
@@ -275,7 +277,11 @@ class FileVolume(qubes.storage.Volume):
                     src_volume, self))
         if self.save_on_stop:
             _remove_if_exists(self.path)
-            copy_file(src_volume.export(), self.path)
+            path = yield from qubes.utils.coro_maybe(src_volume.export())
+            try:
+                copy_file(path, self.path)
+            finally:
+                yield from qubes.utils.coro_maybe(src_volume.export_end(path))
         return self
 
     def import_data(self, size):

qubes/storage/lvm.py

@@ -518,7 +518,8 @@ class ThinVolume(qubes.storage.Volume):
                    self._vid_import.split('/')[1],
                    str(src_volume.size)]
             yield from qubes_lvm_coro(cmd, self.log)
-            src_path = src_volume.export()
+            src_path = yield from qubes.utils.coro_maybe(src_volume.export())
+            try:
                 cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self._vid_import,
                     'conv=sparse', 'status=none', 'bs=128K']
                 if not os.access('/dev/' + self._vid_import, os.W_OK) or \
@@ -527,6 +528,9 @@ class ThinVolume(qubes.storage.Volume):
 
                 p = yield from asyncio.create_subprocess_exec(*cmd)
                 yield from p.wait()
+            finally:
+                yield from qubes.utils.coro_maybe(
+                    src_volume.export_end(src_path))
             if p.returncode != 0:
                 cmd = ['remove', self._vid_import]
                 yield from qubes_lvm_coro(cmd, self.log)

qubes/storage/reflink.py

@@ -35,6 +35,7 @@ import tempfile
 from contextlib import contextmanager, suppress
 
 import qubes.storage
+import qubes.utils
 
 FICLONE = 1074041865        # defined in <linux/fs.h>, assuming sizeof(int)==4
 LOOP_SET_CAPACITY = 0x4C07  # defined in <linux/loop.h>
@@ -297,15 +298,22 @@ class ReflinkVolume(qubes.storage.Volume):
         _import_data_end))
 
     @qubes.storage.Volume.locked
-    @_coroutinized
+    @asyncio.coroutine
     def import_volume(self, src_volume):
         if self.save_on_stop:
             try:
                 success = False
-                _copy_file(src_volume.export(), self._path_import)
+                src_path = yield from qubes.utils.coro_maybe(
+                    src_volume.export())
+                try:
+                    yield from _coroutinized(_copy_file)(
+                        src_path, self._path_import)
+                finally:
+                    yield from qubes.utils.coro_maybe(
+                        src_volume.export_end(src_path))
                 success = True
             finally:
-                self._import_data_end(success)
+                yield from _coroutinized(self._import_data_end)(success)
         return self
 
     def _path_revision(self, number, timestamp=None):

qubes/tests/__init__.py

@@ -251,6 +251,7 @@ def wait_on_fail(func):
                     lambda: asyncio.StreamReaderProtocol(reader),
                     sys.stdin))
             self.loop.run_until_complete(reader.readline())
+            transport.close()
             raise
 
     return wrapper

qubes/tests/integ/backup.py

@@ -36,6 +36,7 @@ import qubes.exc
 import qubes.storage.lvm
 import qubes.tests
 import qubes.tests.storage_lvm
+import qubes.utils
 import qubes.vm
 import qubes.vm.appvm
 import qubes.vm.templatevm
@@ -106,6 +107,13 @@ class BackupTestsMixin(object):
 
         f.close()
 
+    def fill_image_vm(self, vm, volume, size=None, sparse=False):
+        path = self.loop.run_until_complete(vm.storage.export(volume))
+        try:
+            self.fill_image(path, size=size, sparse=sparse)
+        finally:
+            self.loop.run_until_complete(vm.storage.export_end(volume, path))
+
     # NOTE: this was create_basic_vms
     def create_backup_vms(self, pool=None):
         template = self.app.default_template
@@ -120,7 +128,7 @@ class BackupTestsMixin(object):
             testnet.create_on_disk(pool=pool))
         testnet.features['service.ntpd'] = True
         vms.append(testnet)
-        self.fill_image(testnet.storage.export('private'), 20*1024*1024)
+        self.fill_image_vm(testnet, 'private', 20*1024*1024)
 
         vmname = self.make_vm_name('test1')
         self.log.debug("Creating %s" % vmname)
@@ -130,7 +138,7 @@ class BackupTestsMixin(object):
         self.loop.run_until_complete(
             testvm1.create_on_disk(pool=pool))
         vms.append(testvm1)
-        self.fill_image(testvm1.storage.export('private'), 100 * 1024 * 1024)
+        self.fill_image_vm(testvm1, 'private', 100 * 1024 * 1024)
 
         vmname = self.make_vm_name('testhvm1')
         self.log.debug("Creating %s" % vmname)
@@ -140,8 +148,7 @@ class BackupTestsMixin(object):
                                       label='red')
         self.loop.run_until_complete(
             testvm2.create_on_disk(pool=pool))
-        self.fill_image(testvm2.storage.export('root'), 1024 * 1024 * 1024, \
+        self.fill_image_vm(testvm2, 'root', 1024 * 1024 * 1024, True)
-            True)
         vms.append(testvm2)
 
         vmname = self.make_vm_name('template')
@@ -150,7 +157,7 @@ class BackupTestsMixin(object):
             name=vmname, label='red')
         self.loop.run_until_complete(
             testvm3.create_on_disk(pool=pool))
-        self.fill_image(testvm3.storage.export('root'), 100 * 1024 * 1024, True)
+        self.fill_image_vm(testvm3, 'root', 100 * 1024 * 1024, True)
         vms.append(testvm3)
 
         vmname = self.make_vm_name('custom')
@@ -262,11 +269,14 @@ class BackupTestsMixin(object):
             for name, volume in vm.volumes.items():
                 if not volume.rw or not volume.save_on_stop:
                     continue
-                vol_path = volume.export()
+                vol_path = self.loop.run_until_complete(
+                    qubes.utils.coro_maybe(volume.export()))
                 hasher = hashlib.sha1()
                 with open(vol_path, 'rb') as afile:
                     for buf in iter(lambda: afile.read(4096000), b''):
                         hasher.update(buf)
+                self.loop.run_until_complete(
+                    qubes.utils.coro_maybe(volume.export_end(vol_path)))
                 hashes[vm.name][name] = hasher.hexdigest()
         return hashes
 
@@ -382,9 +392,9 @@ class TC_00_Backup(BackupTestsMixin, qubes.tests.SystemTestCase):
             self.fill_image(
                 os.path.join(self.hvmtemplate.dir_path, '00file'),
                 195 * 1024 * 1024 - 4096 * 3)
-            self.fill_image(self.hvmtemplate.storage.export('private'),
+            self.fill_image_vm(self.hvmtemplate, 'private',
                             195 * 1024 * 1024 - 4096 * 3)
-            self.fill_image(self.hvmtemplate.storage.export('root'), 1024 * 1024 * 1024,
+            self.fill_image_vm(self.hvmtemplate, 'root', 1024 * 1024 * 1024,
                             sparse=True)
             vms.append(self.hvmtemplate)
             self.app.save()

qubes/tests/integ/basic.py

@@ -518,9 +518,14 @@ class TC_03_QvmRevertTemplateChanges(qubes.tests.SystemTestCase):
         self.app.save()
 
     def get_rootimg_checksum(self):
-        return subprocess.check_output(
+        path = self.loop.run_until_complete(
-            ['sha1sum', self.test_template.volumes['root'].export()]).\
+            self.test_template.storage.export('root'))
+        try:
+            return subprocess.check_output(['sha1sum', path]).\
                 decode().split(' ')[0]
+        finally:
+            self.loop.run_until_complete(
+                self.test_template.storage.export_end('root', path))
 
     def _do_test(self):
         checksum_before = self.get_rootimg_checksum()

qubes/tests/integ/qrexec.py

@@ -20,6 +20,7 @@
 #
 
 import asyncio
+import contextlib
 import os
 import subprocess
 import sys
@@ -585,6 +586,10 @@ class TC_00_QrexecMixin(object):
         except asyncio.TimeoutError:
             self.fail(
                 "service timeout, probably EOF wasn't transferred from the VM process")
+        finally:
+            with contextlib.suppress(ProcessLookupError):
+                p.terminate()
+            self.loop.run_until_complete(p.wait())
 
         self.assertEqual(stdout, b'test\n',
             'Received data differs from what was expected')
@@ -633,6 +638,10 @@ class TC_00_QrexecMixin(object):
         except asyncio.TimeoutError:
             self.fail(
                 "service timeout, probably EOF wasn't transferred from the VM process")
+        finally:
+            with contextlib.suppress(ProcessLookupError):
+                p.terminate()
+            self.loop.run_until_complete(p.wait())
 
         service_descriptor = b'test.Socket+ test-inst-vm1 keyword adminvm\0'
         self.assertEqual(service_stdout, service_descriptor + b'test1test2',

qubes/tests/integ/vm_qrexec_gui.py

@@ -101,6 +101,8 @@ class TC_00_AppVMMixin(object):
             self.skipTest("Minimal template doesn't have 'gnome-terminal'")
         if 'whonix' in self.template:
             self.skipTest("Whonix template doesn't have 'gnome-terminal'")
+        if 'xfce' in self.template:
+            self.skipTest("Xfce template doesn't have 'gnome-terminal'")
         self.loop.run_until_complete(self.testvm1.start())
         self.assertEqual(self.testvm1.get_power_state(), "Running")
         self.loop.run_until_complete(self.wait_for_session(self.testvm1))

qubes/tests/storage_lvm.py

@@ -40,7 +40,7 @@ from qubes.storage.lvm import ThinPool, ThinVolume, qubes_lvm
 if 'DEFAULT_LVM_POOL' in os.environ.keys():
     DEFAULT_LVM_POOL = os.environ['DEFAULT_LVM_POOL']
 else:
-    DEFAULT_LVM_POOL = 'qubes_dom0/pool00'
+    DEFAULT_LVM_POOL = 'qubes_dom0/vm-pool'
 
 
 def lvm_pool_exists(volume_group, thin_pool):

qubes/tests/vm/appvm.py

@@ -51,11 +51,24 @@ class TestVM(object):
     def is_running(self):
         return self.running
 
+class TestVolume(qubes.storage.Volume):
+    def create(self):
+        pass
+
 class TestPool(qubes.storage.Pool):
+    def __init__(self, *args, **kwargs):
+        super(TestPool, self).__init__(*args, **kwargs)
+        self._volumes = {}
+
     def init_volume(self, vm, volume_config):
         vid = '{}/{}'.format(vm.name, volume_config['name'])
         assert volume_config.pop('pool', None) == self
-        return qubes.storage.Volume(vid=vid, pool=self, **volume_config)
+        vol = TestVolume(vid=vid, pool=self, **volume_config)
+        self._volumes[vid] = vol
+        return vol
+
+    def get_volume(self, vid):
+        return self._volumes[vid]
 
 
 class TC_90_AppVM(qubes.tests.vm.qubesvm.QubesVMTestsMixin,

qubes/tests/vm/dispvm.py

@@ -166,3 +166,28 @@ class TC_00_DispVM(qubes.tests.QubesTestCase):
                 self.app.add_new_vm(qubes.vm.dispvm.DispVM,
                     name='test-dispvm', template=self.appvm)
             self.assertFalse(mock_domains.get_new_unused_dispid.called)
+
+    @mock.patch('os.symlink')
+    @mock.patch('os.makedirs')
+    def test_020_copy_storage_pool(self, mock_makedirs, mock_symlink):
+        self.app.pools['alternative'] = qubes.tests.vm.appvm.TestPool(name='alternative')
+        self.appvm.template_for_dispvms = True
+        self.loop.run_until_complete(self.template.create_on_disk())
+        self.loop.run_until_complete(self.appvm.create_on_disk(pool='alternative'))
+        orig_getitem = self.app.domains.__getitem__
+        with mock.patch.object(self.app, 'domains', wraps=self.app.domains) \
+                as mock_domains:
+            mock_domains.configure_mock(**{
+                'get_new_unused_dispid': mock.Mock(return_value=42),
+                '__getitem__.side_effect': orig_getitem
+            })
+            dispvm = self.app.add_new_vm(qubes.vm.dispvm.DispVM,
+                name='test-dispvm', template=self.appvm)
+            self.loop.run_until_complete(dispvm.create_on_disk())
+        self.assertEqual(dispvm.template, self.appvm)
+        self.assertEqual(dispvm.volumes['private'].pool,
+                         self.appvm.volumes['private'].pool)
+        self.assertEqual(dispvm.volumes['root'].pool,
+                         self.appvm.volumes['root'].pool)
+        self.assertEqual(dispvm.volumes['volatile'].pool,
+                         self.appvm.volumes['volatile'].pool)

qubes/vm/dispvm.py

@@ -109,6 +109,12 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
                     self.volume_config[name] = config.copy()
                     if 'vid' in self.volume_config[name]:
                         del self.volume_config[name]['vid']
+                # copy pool setting from base AppVM; root and private would be
+                # in the same pool anyway (because of snap_on_start),
+                # but not volatile, which could be surprising
+                elif 'pool' not in self.volume_config[name] \
+                        and 'pool' in config:
+                    self.volume_config[name]['pool'] = config['pool']
 
         super(DispVM, self).__init__(app, xml, *args, **kwargs)