storage/file: major FilePool/FileVolume cleanup and documentation

This driver isn't used in default Qubes 4.0 installation, but if we do
have it, let it follow defined API and its own documentation. And also
explicitly reject not supported operations:
 - support only revisions_to_keep<=1, but do not support revert() anyway
 (implemented version were wrong on so many levels...)
 - use 'save_on_stop'/'snap_on_start' properties directly instead of
 obsolete volume types
 - don't call sudo - qubesd is running as root
 - consistently use path, path_cow, path_source, path_source_cow

Also, add tests for BlockDevice instance returned by
FileVolume.block_device().

QubesOS/qubes-issues#2256

qubes/storage/__init__.py

@@ -111,6 +111,15 @@ class Volume(object):
         assert source is None or (isinstance(source, Volume)
                                   and source.pool == pool)
 
+        if snap_on_start and source is None:
+            msg = "snap_on_start specified on {!r} but no volume source set"
+            msg = msg.format(name)
+            raise StoragePoolException(msg)
+        elif not snap_on_start and source is not None:
+            msg = "source specified on {!r} but no snap_on_start set"
+            msg = msg.format(name)
+            raise StoragePoolException(msg)
+
         #: Name of the volume in a domain it's attached to (like `root` or
         #: `private`).
         self.name = str(name)

qubes/storage/file.py

@@ -37,6 +37,20 @@ BLKSIZE = 512
 
 class FilePool(qubes.storage.Pool):
     ''' File based 'original' disk implementation
+
+    Volumes are stored in sparse files. Additionally device-mapper is used for
+    applying copy-on-write layer.
+
+    Quick reference on device-mapper layers:
+
+    snap_on_start save_on_stop layout
+    yes           yes          not supported
+    no            yes          snapshot-origin(volume.img, volume-cow.img)
+    yes           no           snapshot(
+                                   snapshot(source.img, source-cow.img),
+                                   volume-cow.img)
+    no            no           volume.img directly
+
     '''  # pylint: disable=protected-access
     driver = 'file'
 
@@ -57,16 +71,18 @@ class FilePool(qubes.storage.Pool):
         }
 
     def init_volume(self, vm, volume_config):
+        if volume_config.get('snap_on_start', False) and \
+                volume_config.get('save_on_stop', False):
+            raise NotImplementedError(
+                'snap_on_start + save_on_stop not supported by file driver')
         volume_config['dir_path'] = self.dir_path
-        if os.path.join(self.dir_path, self._vid_prefix(vm)) == vm.dir_path:
-            volume_config['backward_comp'] = True
 
         if 'vid' not in volume_config:
             volume_config['vid'] = os.path.join(
                 self._vid_prefix(vm), volume_config['name'])
 
         try:
-            if volume_config['reset_on_start']:
+            if not volume_config.get('save_on_stop', False):
                 volume_config['revisions_to_keep'] = 0
         except KeyError:
             pass
@@ -74,6 +90,10 @@ class FilePool(qubes.storage.Pool):
             if 'revisions_to_keep' not in volume_config:
                 volume_config['revisions_to_keep'] = self.revisions_to_keep
 
+        if int(volume_config['revisions_to_keep']) > 1:
+            raise NotImplementedError(
+                'FilePool supports maximum 1 volume revision to keep')
+
         volume_config['pool'] = self
         volume = FileVolume(**volume_config)
         self._volumes += [volume]
@@ -130,59 +150,37 @@ class FileVolume(qubes.storage.Volume):
     ''' Parent class for the xen volumes implementation which expects a
         `target_dir` param on initialization.  '''
 
-    def __init__(self, dir_path, backward_comp=False, **kwargs):
+    def __init__(self, dir_path, **kwargs):
         self.dir_path = dir_path
-        self.backward_comp = backward_comp
         assert self.dir_path, "dir_path not specified"
         super(FileVolume, self).__init__(**kwargs)
 
-        if self.snap_on_start and self.source is None:
+        if self.snap_on_start:
-            msg = "snap_on_start specified on {!r} but no volume source set"
-            msg = msg.format(self.name)
-            raise qubes.storage.StoragePoolException(msg)
-        elif not self.snap_on_start and self.source is not None:
-            msg = "source specified on {!r} but no snap_on_start set"
-            msg = msg.format(self.name)
-            raise qubes.storage.StoragePoolException(msg)
-
-        if self._is_snapshot:
             img_name = self.source.vid + '-cow.img'
             self.path_source_cow = os.path.join(self.dir_path, img_name)
-        elif self._is_volume or self._is_volatile:
-            pass
-        elif self._is_origin:
-            pass
-        else:
-            assert False, 'This should not happen'
 
     def create(self):
         assert isinstance(self.size, int) and self.size > 0, \
-            'Volatile volume size must be > 0'
+            'Volume size must be > 0'
-        if self._is_origin:
+        if not self.snap_on_start:
             create_sparse_file(self.path, self.size)
+        # path_cow not needed only in volatile volume
+        if self.save_on_stop or self.snap_on_start:
             create_sparse_file(self.path_cow, self.size)
-        elif not self._is_snapshot:
-            if self.source is not None:
-                source_path = os.path.join(self.dir_path,
-                    self.source.vid + '.img')
-                copy_file(source_path, self.path)
-            elif self._is_volatile:
-                pass
-            else:
-                create_sparse_file(self.path, self.size)
 
     def remove(self):
-        if not self.internal:
+        if not self.snap_on_start:
-            return  # do not remove random attached file volumes
-        elif self._is_snapshot:
-            return  # no need to remove, because it's just a snapshot
-        else:
             _remove_if_exists(self.path)
-            if self._is_origin:
+        if self.snap_on_start or self.save_on_stop:
-                _remove_if_exists(self.path_cow)
+            _remove_if_exists(self.path_cow)
 
     def is_dirty(self):
-        return False  # TODO: How to implement this?
+        if not self.save_on_stop:
+            return False
+        if os.path.exists(self.path_cow):
+            stat = os.stat(self.path_cow)
+            return stat.st_blocks > 0
+        return False
 
     def resize(self, size):
         ''' Expands volume, throws
@@ -203,7 +201,7 @@ class FileVolume(qubes.storage.Volume):
         with open(self.path, 'a+b') as fd:
             fd.truncate(size)
 
-        p = subprocess.Popen(['sudo', 'losetup', '--associated', self.path],
+        p = subprocess.Popen(['losetup', '--associated', self.path],
                              stdout=subprocess.PIPE)
         result = p.communicate()
 
@@ -212,28 +210,27 @@ class FileVolume(qubes.storage.Volume):
             loop_dev = m.group(1)
 
             # resize loop device
-            subprocess.check_call(['sudo', 'losetup', '--set-capacity',
+            subprocess.check_call(['losetup', '--set-capacity',
                                    loop_dev])
         self.size = size
 
     def commit(self):
         msg = 'Tried to commit a non commitable volume {!r}'.format(self)
-        assert (self._is_origin or self._is_volume) and self.rw, msg
+        assert self.save_on_stop and self.rw, msg
-
-        if self._is_volume:
-            return self
 
         if os.path.exists(self.path_cow):
-            old_path = self.path_cow + '.old'
+            if self.revisions_to_keep:
-            os.rename(self.path_cow, old_path)
+                old_path = self.path_cow + '.old'
+                os.rename(self.path_cow, old_path)
+            else:
+                os.unlink(self.path_cow)
 
-        old_umask = os.umask(0o002)
+        create_sparse_file(self.path_cow, self.size)
-        with open(self.path_cow, 'w') as f_cow:
-            f_cow.truncate(self.size)
-        os.umask(old_umask)
         return self
 
     def export(self):
+        # FIXME: this should rather return snapshot(self.path, self.path_cow)
+        #  if domain is running
         return self.path
 
     def import_volume(self, src_volume):
@@ -251,53 +248,30 @@ class FileVolume(qubes.storage.Volume):
 
     def reset(self):
         ''' Remove and recreate a volatile volume '''
-        assert self._is_volatile, "Not a volatile volume"
+        assert not self.snap_on_start and not self.save_on_stop, \
+            "Not a volatile volume"
         assert isinstance(self.size, int) and self.size > 0, \
             'Volatile volume size must be > 0'
 
         _remove_if_exists(self.path)
-
+        create_sparse_file(self.path, self.size)
-        with open(self.path, "w") as f_volatile:
-            f_volatile.truncate(self.size)
         return self
 
-    def revert(self, revision=None):
-        if revision is not None:
-            try:
-                return self.revisions[revision]
-            except KeyError:
-                msg = "Volume {!r} does not have revision {!s}"
-                msg = msg.format(self, revision)
-                raise qubes.storage.StoragePoolException(msg)
-        else:
-            try:
-                old_path = self.revisions.values().pop()
-                os.rename(old_path, self.path_cow)
-            except IndexError:
-                msg = "Volume {!r} does not have old revisions".format(self)
-                raise qubes.storage.StoragePoolException(msg)
-
     def start(self):
-        if self._is_volatile:
+        if not self.save_on_stop and not self.snap_on_start:
             self.reset()
         else:
-            _check_path(self.path)
+            if not self.save_on_stop:
-            if self.snap_on_start:
+                # make sure previous snapshot is removed - even if VM
-                if not self.save_on_stop:
+                # shutdown routine wasn't called (power interrupt or so)
-                    # make sure previous snapshot is removed - even if VM
+                _remove_if_exists(self.path_cow)
-                    # shutdown routing wasn't called (power interrupt or so)
+            if not os.path.exists(self.path_cow):
-                    _remove_if_exists(self.path_cow)
+                create_sparse_file(self.path_cow, self.size)
-                try:
+            if not self.snap_on_start:
-                    _check_path(self.path_cow)
+                _check_path(self.path)
-                except qubes.storage.StoragePoolException:
+            if hasattr(self, 'path_source_cow'):
-                    create_sparse_file(self.path_cow, self.size)
+                if not os.path.exists(self.path_source_cow):
-                    _check_path(self.path_cow)
+                    create_sparse_file(self.path_source_cow, self.size)
-                if hasattr(self, 'path_source_cow'):
-                    try:
-                        _check_path(self.path_source_cow)
-                    except qubes.storage.StoragePoolException:
-                        create_sparse_file(self.path_source_cow, self.size)
-                        _check_path(self.path_source_cow)
         return self
 
     def stop(self):
@@ -311,7 +285,7 @@ class FileVolume(qubes.storage.Volume):
 
     @property
     def path(self):
-        if self._is_snapshot:
+        if self.snap_on_start:
             return os.path.join(self.dir_path, self.source.vid + '.img')
         return os.path.join(self.dir_path, self.vid + '.img')
 
@@ -322,18 +296,19 @@ class FileVolume(qubes.storage.Volume):
 
     def verify(self):
         ''' Verifies the volume. '''
-        if not os.path.exists(self.path) and not self._is_volatile:
+        if not os.path.exists(self.path) and \
+                (self.snap_on_start or self.save_on_stop):
             msg = 'Missing image file: {!s}.'.format(self.path)
             raise qubes.storage.StoragePoolException(msg)
         return True
 
     @property
     def script(self):
-        if self._is_volume or self._is_volatile:
+        if not self.snap_on_start and not self.save_on_stop:
             return None
-        elif self._is_origin:
+        elif not self.snap_on_start and self.save_on_stop:
             return 'block-origin'
-        elif self._is_origin_snapshot or self._is_snapshot:
+        elif self.snap_on_start:
             return 'block-snapshot'
 
     def block_device(self):
@@ -341,9 +316,9 @@ class FileVolume(qubes.storage.Volume):
             the libvirt XML template as <disk>.
         '''
         path = self.path
-        if self._is_snapshot:
+        if self.snap_on_start:
             path += ":" + self.path_source_cow
-        if self._is_origin or self._is_snapshot:
+        if self.snap_on_start or self.save_on_stop:
             path += ":" + self.path_cow
         return qubes.storage.BlockDevice(path, self.name, self.script, self.rw,
                                          self.domain, self.devtype)
@@ -360,39 +335,13 @@ class FileVolume(qubes.storage.Volume):
 
         seconds = os.path.getctime(old_revision)
         iso_date = qubes.storage.isodate(seconds).split('.', 1)[0]
-        return {iso_date: old_revision}
+        return {'old': iso_date}
 
     @property
     def usage(self):
         ''' Returns the actualy used space '''
         return get_disk_usage(self.vid)
 
-    @property
-    def _is_volatile(self):
-        ''' Internal helper. Useful for differentiating volume handling '''
-        return not self.snap_on_start and not self.save_on_stop
-
-    @property
-    def _is_origin(self):
-        ''' Internal helper. Useful for differentiating volume handling '''
-        # pylint: disable=line-too-long
-        return self.save_on_stop and self.revisions_to_keep > 0  # NOQA
-
-    @property
-    def _is_snapshot(self):
-        ''' Internal helper. Useful for differentiating volume handling '''
-        return self.snap_on_start and not self.save_on_stop
-
-    @property
-    def _is_origin_snapshot(self):
-        ''' Internal helper. Useful for differentiating volume handling '''
-        return self.snap_on_start and self.save_on_stop
-
-    @property
-    def _is_volume(self):
-        ''' Internal helper. Usefull for differentiating volume handling '''
-        # pylint: disable=line-too-long
-        return not self.snap_on_start and self.save_on_stop and self.revisions_to_keep == 0  # NOQA
 
 def create_sparse_file(path, size):
     ''' Create an empty sparse file '''

qubes/tests/storage_file.py

@@ -136,6 +136,15 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
         self.assertFalse(volume.snap_on_start)
         self.assertTrue(volume.save_on_stop)
         self.assertTrue(volume.rw)
+        block = volume.block_device()
+        self.assertEqual(block.path,
+            '{base}.img:{base}-cow.img'.format(
+                base=self.POOL_DIR + '/appvms/' + vm.name + '/root'))
+        self.assertEqual(block.script, 'block-origin')
+        self.assertEqual(block.rw, True)
+        self.assertEqual(block.name, 'root')
+        self.assertEqual(block.devtype, 'disk')
+        self.assertIsNone(block.domain)
 
     def test_001_snapshot_volume(self):
         template_vm = self.app.default_template
@@ -169,6 +178,19 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
         self.assertFalse(volume.save_on_stop)
         self.assertFalse(volume.rw)
         self.assertEqual(volume.usage, 0)
+        block = volume.block_device()
+        assert isinstance(block, qubes.storage.BlockDevice)
+        self.assertEqual(block.path,
+            '{base}/{src}.img:{base}/{src}-cow.img:'
+            '{base}/{dst}-cow.img'.format(
+                base=self.POOL_DIR,
+                src='vm-templates/' + template_vm.name + '/root',
+                dst='appvms/' + vm.name + '/root',
+        ))
+        self.assertEqual(block.name, 'root')
+        self.assertEqual(block.script, 'block-snapshot')
+        self.assertEqual(block.rw, False)
+        self.assertEqual(block.devtype, 'disk')
 
     def test_002_read_write_volume(self):
         config = {
@@ -186,6 +208,12 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
         self.assertFalse(volume.snap_on_start)
         self.assertTrue(volume.save_on_stop)
         self.assertTrue(volume.rw)
+        block = volume.block_device()
+        self.assertEqual(block.name, 'root')
+        self.assertEqual(block.path, '{base}.img:{base}-cow.img'.format(
+            base=self.POOL_DIR + '/appvms/' + vm.name + '/root'))
+        self.assertEqual(block.rw, True)
+        self.assertEqual(block.script, 'block-origin')
 
     def test_003_read_only_volume(self):
         template = self.app.default_template
@@ -207,6 +235,8 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
         self.assertFalse(volume.snap_on_start)
         self.assertFalse(volume.save_on_stop)
         self.assertFalse(volume.rw)
+        block = volume.block_device()
+        self.assertEqual(block.rw, False)
 
     def test_004_volatile_volume(self):
         config = {
@@ -248,7 +278,8 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
 
         expected = vm.dir_path + '/root.img:' + vm.dir_path + '/root-cow.img'
         self.assertVolumePath(vm, 'root', expected, rw=True)
-        expected = vm.dir_path + '/private.img'
+        expected = vm.dir_path + '/private.img:' + \
+                   vm.dir_path + '/private-cow.img'
         self.assertVolumePath(vm, 'private', expected, rw=True)
         expected = vm.dir_path + '/volatile.img'
         self.assertVolumePath(vm, 'volatile', expected, rw=True)