From dd1e05dc839ae6f52becf71b746b02d78d76a8cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 30 Jun 2017 21:25:37 +0200
Subject: [PATCH 01/12] vm: drop rename related methods

Since VM name in immutable now, drop rename-related methods.

QubesOS/qubes-issues#2868
---
 qubes/tests/integ/basic.py | 62 ------------------------------------
 qubes/tests/vm/qubesvm.py  | 44 --------------------------
 qubes/vm/qubesvm.py        | 65 --------------------------------------
 3 files changed, 171 deletions(-)

diff --git a/qubes/tests/integ/basic.py b/qubes/tests/integ/basic.py
index ebf9f6d4..642f20d0 100644
--- a/qubes/tests/integ/basic.py
+++ b/qubes/tests/integ/basic.py
@@ -77,68 +77,6 @@ class TC_01_Properties(qubes.tests.SystemTestsMixin, qubes.tests.QubesTestCase):
                                       label='red')
         self.loop.run_until_complete(self.vm.create_on_disk())
 
-    @unittest.expectedFailure
-    def test_000_rename(self):
-        newname = self.make_vm_name('newname')
-
-        self.assertEqual(self.vm.name, self.vmname)
-        self.vm.firewall.policy = 'drop'
-        self.vm.firewall.rules = [
-            qubes.firewall.Rule(None, action='accept', specialtarget='dns')
-        ]
-        self.vm.firewall.save()
-        self.vm.autostart = True
-        self.addCleanup(os.system,
-                        'sudo systemctl -q disable qubes-vm@{}.service || :'.
-                        format(self.vmname))
-        pre_rename_firewall = self.vm.firewall.rules
-
-        with self.assertNotRaises(
-                (OSError, libvirt.libvirtError, qubes.exc.QubesException)):
-            self.vm.name = newname
-        self.assertEqual(self.vm.name, newname)
-        self.assertEqual(self.vm.dir_path,
-            os.path.join(
-                qubes.config.system_path['qubes_base_dir'],
-                qubes.config.system_path['qubes_appvms_dir'], newname))
-        self.assertTrue(os.path.exists(
-            os.path.join(self.vm.dir_path, "apps", newname + "-vm.directory")))
-        # FIXME: set whitelisted-appmenus.list first
-        self.assertTrue(os.path.exists(os.path.join(
-            self.vm.dir_path, "apps", newname + "-firefox.desktop")))
-        self.assertTrue(os.path.exists(
-            os.path.join(os.getenv("HOME"), ".local/share/desktop-directories",
-                newname + "-vm.directory")))
-        self.assertTrue(os.path.exists(
-            os.path.join(os.getenv("HOME"), ".local/share/applications",
-                newname + "-firefox.desktop")))
-        self.assertFalse(os.path.exists(
-            os.path.join(os.getenv("HOME"), ".local/share/desktop-directories",
-                self.vmname + "-vm.directory")))
-        self.assertFalse(os.path.exists(
-            os.path.join(os.getenv("HOME"), ".local/share/applications",
-                self.vmname + "-firefox.desktop")))
-        self.vm.firewall.load()
-        self.assertEqual(pre_rename_firewall, self.vm.firewall.rules)
-        with self.assertNotRaises((qubes.exc.QubesException, OSError)):
-            self.vm.firewall.save()
-        self.assertTrue(self.vm.autostart)
-        self.assertTrue(os.path.exists(
-            '/etc/systemd/system/multi-user.target.wants/'
-            'qubes-vm@{}.service'.format(newname)))
-        self.assertFalse(os.path.exists(
-            '/etc/systemd/system/multi-user.target.wants/'
-            'qubes-vm@{}.service'.format(self.vmname)))
-
-    def test_001_rename_libvirt_undefined(self):
-        self.vm.libvirt_domain.undefine()
-        self.vm._libvirt_domain = None  # pylint: disable=protected-access
-
-        newname = self.make_vm_name('newname')
-        with self.assertNotRaises(
-                (OSError, libvirt.libvirtError, qubes.exc.QubesException)):
-            self.vm.name = newname
-
     @unittest.expectedFailure
     def test_030_clone(self):
         testvm1 = self.app.add_new_vm(
diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py
index cb006e34..60535d44 100644
--- a/qubes/tests/vm/qubesvm.py
+++ b/qubes/tests/vm/qubesvm.py
@@ -80,50 +80,6 @@ class TC_00_setters(qubes.tests.QubesTestCase):
             qubes.vm.qubesvm._setter_qid(self.vm,
                 self.prop, qubes.config.max_qid + 5)
 
-
-    def test_010_setter_name(self):
-        self.assertEqual(
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, 'test_name-1'),
-            'test_name-1')
-
-    def test_011_setter_name_not_a_string(self):
-        # pylint: disable=invalid-name
-        with self.assertRaises(TypeError):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, False)
-
-    def test_012_setter_name_longer_than_31(self):
-        # pylint: disable=invalid-name
-        with self.assertRaises(ValueError):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, 't' * 32)
-
-    def test_013_setter_name_illegal_character(self):
-        # pylint: disable=invalid-name
-        with self.assertRaises(ValueError):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, 'test#')
-
-    def test_014_setter_name_first_not_letter(self):
-        # pylint: disable=invalid-name
-        with self.assertRaises(ValueError):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, '1test')
-
-    def test_015_setter_name_running(self):
-        self.vm.running = True
-        with self.assertRaises(qubes.exc.QubesVMNotHaltedError):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, 'testname')
-
-    def test_016_setter_name_installed_by_rpm(self):
-        # pylint: disable=invalid-name
-        self.vm.installed_by_rpm = True
-        with self.assertRaises(qubes.exc.QubesException):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, 'testname')
-
-    def test_017_setter_name_duplicate(self):
-        # pylint: disable=invalid-name
-        self.vm.app.domains['duplicate'] = TestVM(name='duplicate')
-        with self.assertRaises(qubes.exc.QubesException):
-            qubes.vm.qubesvm._setter_name(self.vm, self.prop, 'duplicate')
-
-
     @unittest.skip('test not implemented')
     def test_020_setter_kernel(self):
         pass
diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index f9e84660..a96288dd 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -72,28 +72,6 @@ def _setter_qid(self, prop, value):
     return value
 
 
-def _setter_name(self, prop, value):
-    ''' Helper for setting the domain name '''
-    qubes.vm.validate_name(self, prop, value)
-
-    if self.is_running():
-        raise qubes.exc.QubesVMNotHaltedError(
-            self, 'Cannot change name of running VM')
-
-    try:
-        if self.installed_by_rpm:
-            raise qubes.exc.QubesException('Cannot rename VM installed by RPM '
-                '-- first clone VM and then use yum to remove package.')
-    except AttributeError:
-        pass
-
-    if value in self.app.domains:
-        raise qubes.exc.QubesPropertyValueError(self, prop, value,
-            'VM named {} alread exists'.format(value))
-
-    return value
-
-
 def _setter_kernel(self, prop, value):
     ''' Helper for setting the domain kernel and running sanity checks on it.
     '''  # pylint: disable=unused-argument
@@ -732,26 +710,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
             else:
                 shutil.copy(newvalue.icon_path, self.icon_path)
 
-    @qubes.events.handler('property-pre-set:name')
-    def on_property_pre_set_name(self, event, name, newvalue, oldvalue=None):
-        # pylint: disable=unused-argument
-        try:
-            self.app.domains[newvalue]
-        except KeyError:
-            pass
-        else:
-            raise qubes.exc.QubesValueError(
-                'VM named {!r} already exists'.format(newvalue))
-
-        # TODO not self.is_stopped() would be more appropriate
-        if self.is_running():
-            raise qubes.exc.QubesVMNotHaltedError(
-                'Cannot change name of running domain {!r}'.format(oldvalue))
-
-        if self.autostart:
-            subprocess.check_call(['sudo', 'systemctl', '-q', 'disable',
-                                   'qubes-vm@{}.service'.format(oldvalue)])
-
     @qubes.events.handler('property-pre-set:kernel')
     def on_property_pre_set_kernel(self, event, name, newvalue, oldvalue=None):
         # pylint: disable=unused-argument
@@ -772,29 +730,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
                     'Kernel {!r} not properly installed: '
                     'missing {!r} file'.format(newvalue, filename))
 
-    @qubes.events.handler('property-set:name')
-    def on_property_set_name(self, event, name, newvalue, oldvalue=None):
-        # pylint: disable=unused-argument
-        self.init_log()
-
-        old_dir_path = os.path.join(os.path.dirname(self.dir_path), oldvalue)
-        new_dir_path = os.path.join(os.path.dirname(self.dir_path), newvalue)
-        os.rename(old_dir_path, new_dir_path)
-
-        self.storage.rename(oldvalue, newvalue)
-
-        if self._libvirt_domain is not None:
-            self.libvirt_domain.undefine()
-            self._libvirt_domain = None
-        if self._qdb_connection is not None:
-            self._qdb_connection.close()
-            self._qdb_connection = None
-
-        self._update_libvirt_domain()
-
-        if self.autostart:
-            self.autostart = self.autostart
-
     @qubes.events.handler('property-pre-set:autostart')
     def on_property_pre_set_autostart(self, event, name, newvalue,
             oldvalue=None):

From 697eb05c20cc5d4a7f7612c0fc65afe2138d34c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Fri, 30 Jun 2017 21:26:45 +0200
Subject: [PATCH 02/12] storage: drop rename support

Since VM name is immutable, rename method can be dropped from storage
API.

QubesOS/qubes-issues#2868
---
 qubes/storage/__init__.py | 11 -----------
 qubes/storage/file.py     | 20 --------------------
 qubes/storage/kernels.py  |  3 ---
 qubes/storage/lvm.py      | 17 -----------------
 4 files changed, 51 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index 22123b81..5cd647be 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -486,13 +486,6 @@ class Storage(object):
 
         return result
 
-    def rename(self, old_name, new_name):
-        ''' Notify the pools that the domain was renamed '''
-        volumes = self.vm.volumes
-        for name, volume in volumes.items():
-            pool = volume.pool
-            volumes[name] = pool.rename(volume, old_name, new_name)
-
     @asyncio.coroutine
     def verify(self):
         '''Verify that the storage is sane.
@@ -722,10 +715,6 @@ class Pool(object):
         '''
         raise self._not_implemented("init_volume")
 
-    def rename(self, volume, old_name, new_name):
-        ''' Called when the domain changes its name '''
-        raise self._not_implemented("rename")
-
     def setup(self):
         ''' Called when adding a pool to the system. Use this for implementation
             specific set up.
diff --git a/qubes/storage/file.py b/qubes/storage/file.py
index 5b352254..f683114e 100644
--- a/qubes/storage/file.py
+++ b/qubes/storage/file.py
@@ -79,26 +79,6 @@ class FilePool(qubes.storage.Pool):
         self._volumes += [volume]
         return volume
 
-    def rename(self, volume, old_name, new_name):
-        assert issubclass(volume.__class__, FileVolume)
-        subdir, _, volume_path = volume.vid.split('/', 2)
-
-        if volume._is_origin:
-            # TODO: Renaming the old revisions
-            new_path = os.path.join(self.dir_path, subdir, new_name)
-            if not os.path.exists(new_path):
-                os.mkdir(new_path, 0o755)
-            new_volume_path = os.path.join(new_path, self.name + '.img')
-            if not volume.backward_comp:
-                os.rename(volume.path, new_volume_path)
-            new_volume_path_cow = os.path.join(new_path, self.name + '-cow.img')
-            if os.path.exists(new_volume_path_cow) and not volume.backward_comp:
-                os.rename(volume.path_cow, new_volume_path_cow)
-
-        volume.vid = os.path.join(subdir, new_name, volume_path)
-
-        return volume
-
     def destroy(self):
         pass
 
diff --git a/qubes/storage/kernels.py b/qubes/storage/kernels.py
index 1f965e92..60d304eb 100644
--- a/qubes/storage/kernels.py
+++ b/qubes/storage/kernels.py
@@ -157,9 +157,6 @@ class LinuxKernel(Pool):
     def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
         pass
 
-    def rename(self, volume, old_name, new_name):
-        return volume
-
     def setup(self):
         pass
 
diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py
index 77f94d0c..751daf60 100644
--- a/qubes/storage/lvm.py
+++ b/qubes/storage/lvm.py
@@ -88,23 +88,6 @@ class ThinPool(qubes.storage.Pool):
         volume_config['pool'] = self
         return ThinVolume(**volume_config)
 
-    def rename(self, volume, old_name, new_name):
-        ''' Called when the domain changes its name '''
-        new_vid = "{!s}/vm-{!s}-{!s}".format(self.volume_group, new_name,
-                                          volume.name)
-        if volume.save_on_stop:
-            cmd = ['clone', volume.vid, new_vid]
-            qubes_lvm(cmd, self.log)
-            cmd = ['remove', volume.vid]
-            qubes_lvm(cmd, self.log)
-
-        volume.vid = new_vid
-
-        if volume.snap_on_start:
-            volume._vid_snap = volume.vid + '-snap'
-        reset_cache()
-        return volume
-
     def setup(self):
         pass  # TODO Should we create a non existing pool?
 

From 820539e909a4162bf5493aa2138b0c54f4fae2a3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 1 Jul 2017 20:47:08 +0200
Subject: [PATCH 03/12] storage: make volume snap_on_start/save_on_stop
 explicit

Always define those properties, always include them in volume config.
Also simplify overriding pool based on volume type defined by those:
override pool unless snap_on_start=True.

QubesOS/qubes-issues#2256
---
 qubes/storage/__init__.py | 30 +++++++++++-------------------
 qubes/vm/appvm.py         |  4 ++++
 qubes/vm/dispvm.py        |  4 ++++
 qubes/vm/qubesvm.py       | 13 +++++--------
 qubes/vm/standalonevm.py  |  4 ++++
 qubes/vm/templatevm.py    |  4 ++++
 6 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index 5cd647be..26598f85 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -262,29 +262,21 @@ class Volume(object):
     @property
     def config(self):
         ''' return config data for serialization to qubes.xml '''
-        result = {'name': self.name, 'pool': str(self.pool), 'vid': self.vid, }
-
-        if self.internal:
-            result['internal'] = self.internal
-
-        if self.removable:
-            result['removable'] = self.removable
-
-        if self.revisions_to_keep:
-            result['revisions_to_keep'] = self.revisions_to_keep
-
-        if self.rw:
-            result['rw'] = self.rw
-
-        if self.save_on_stop:
-            result['save_on_stop'] = self.save_on_stop
+        result = {
+            'name': self.name,
+            'pool': str(self.pool),
+            'vid': self.vid,
+            'internal': self.internal,
+            'removable': self.removable,
+            'revisions_to_keep': self.revisions_to_keep,
+            'rw': self.rw,
+            'save_on_stop': self.save_on_stop,
+            'snap_on_start': self.snap_on_start,
+        }
 
         if self.size:
             result['size'] = self.size
 
-        if self.snap_on_start:
-            result['snap_on_start'] = self.snap_on_start
-
         if self.source:
             result['source'] = str(self.source)
 
diff --git a/qubes/vm/appvm.py b/qubes/vm/appvm.py
index 74ddc28e..b818c995 100644
--- a/qubes/vm/appvm.py
+++ b/qubes/vm/appvm.py
@@ -63,6 +63,8 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
             'volatile': {
                 'name': 'volatile',
                 'pool': 'default',
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'size': defaults['root_img_size'],
                 'internal': True,
                 'rw': True,
@@ -70,6 +72,8 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
             'kernel': {
                 'name': 'kernel',
                 'pool': 'linux-kernel',
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'rw': False,
                 'internal': True
             }
diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py
index ff8e39d5..8586627b 100644
--- a/qubes/vm/dispvm.py
+++ b/qubes/vm/dispvm.py
@@ -61,6 +61,8 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
                 'name': 'volatile',
                 'pool': 'default',
                 'internal': True,
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'rw': True,
                 'size': qubes.config.defaults['root_img_size'] +
                         qubes.config.defaults['private_img_size'],
@@ -68,6 +70,8 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
             'kernel': {
                 'name': 'kernel',
                 'pool': 'linux-kernel',
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'rw': False,
                 'internal': True
             }
diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index a96288dd..2c0ea441 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -1824,20 +1824,17 @@ def _clean_volume_config(config):
 
 def _patch_pool_config(config, pool=None, pools=None):
     assert pool is not None or pools is not None
-    is_saveable = 'save_on_stop' in config and config['save_on_stop']
-    is_resetable = not ('snap_on_start' in config and  # volatile
-                        config['snap_on_start'] and not is_saveable)
-
-    is_exportable = is_saveable or is_resetable
+    is_snapshot = config['snap_on_start']
+    is_rw = config['rw']
 
     name = config['name']
 
-    if pool and is_exportable and config['pool'] == 'default':
+    if pool and not is_snapshot and is_rw:
         config['pool'] = str(pool)
-    elif pool and not is_exportable:
+    elif pool:
         pass
     elif pools and name in pools.keys():
-        if is_exportable:
+        if not is_snapshot:
             config['pool'] = str(pools[name])
         else:
             msg = "Can't clone a snapshot volume {!s} to pool {!s} " \
diff --git a/qubes/vm/standalonevm.py b/qubes/vm/standalonevm.py
index ef3b59d1..7aab434d 100644
--- a/qubes/vm/standalonevm.py
+++ b/qubes/vm/standalonevm.py
@@ -51,6 +51,8 @@ class StandaloneVM(qubes.vm.qubesvm.QubesVM):
             'volatile': {
                 'name': 'volatile',
                 'pool': 'default',
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'internal': True,
                 'rw': True,
                 'size': qubes.config.defaults['root_img_size'],
@@ -58,6 +60,8 @@ class StandaloneVM(qubes.vm.qubesvm.QubesVM):
             'kernel': {
                 'name': 'kernel',
                 'pool': 'linux-kernel',
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'rw': False,
                 'internal': True
             }
diff --git a/qubes/vm/templatevm.py b/qubes/vm/templatevm.py
index 7be3e0a2..8cd07528 100644
--- a/qubes/vm/templatevm.py
+++ b/qubes/vm/templatevm.py
@@ -88,12 +88,16 @@ class TemplateVM(QubesVM):
                 'name': 'volatile',
                 'pool': 'default',
                 'size': defaults['root_img_size'],
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'internal': True,
                 'rw': True,
             },
             'kernel': {
                 'name': 'kernel',
                 'pool': 'linux-kernel',
+                'snap_on_start': False,
+                'save_on_stop': False,
                 'internal': True,
                 'rw': False
             }

From 59718736801ea1b97094649cb94d529dfaa809fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 1 Jul 2017 21:20:35 +0200
Subject: [PATCH 04/12] storage: drop functions not being part of the API

commit/recover/reset should really be handled in start/stop. Nothing
stops specific pool implementation to define such functions privately.

QubesOS/qubes-issues#2256
---
 qubes/storage/__init__.py | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index 26598f85..2e7942a8 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -152,12 +152,6 @@ class Volume(object):
         This can be implemented as a coroutine.'''
         raise self._not_implemented("remove")
 
-    def commit(self):
-        ''' Write the snapshot to disk
-
-        This can be implemented as a coroutine.'''
-        raise self._not_implemented("commit")
-
     def export(self):
         ''' Returns an object that can be `open()`. '''
         raise self._not_implemented("export")
@@ -195,15 +189,6 @@ class Volume(object):
         '''
         raise self._not_implemented("is_outdated")
 
-    def recover(self):
-        ''' Try to recover a :py:class:`Volume` or :py:class:`SnapVolume` '''
-        raise self._not_implemented("recover")
-
-    def reset(self):
-        ''' Drop and recreate volume without copying it's content from source.
-        '''
-        raise self._not_implemented("reset")
-
     def resize(self, size):
         ''' Expands volume, throws
             :py:class:`qubes.storage.StoragePoolException` if
@@ -544,19 +529,6 @@ class Storage(object):
         if futures:
             yield from asyncio.wait(futures)
 
-    @asyncio.coroutine
-    def commit(self):
-        ''' Makes changes to an 'origin' volume persistent '''
-        futures = []
-        for volume in self.vm.volumes.values():
-            if volume.save_on_stop:
-                ret = volume.commit()
-                if asyncio.iscoroutine(ret):
-                    futures.append(ret)
-
-        if futures:
-            yield asyncio.wait(futures)
-
     def unused_frontend(self):
         ''' Find an unused device name '''
         unused_frontends = self.AVAILABLE_FRONTENDS.difference(

From 82c3f85042dfc057482e9a97392cdafb24089626 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 1 Jul 2017 21:27:56 +0200
Subject: [PATCH 05/12] storage: add API documentation

QubesOS/qubes-issues#2256
---
 doc/index.rst             |   1 +
 doc/qubes-storage.rst     | 145 ++++++++++++++++++++++++++++++++++++++
 qubes/storage/__init__.py |  87 +++++++++++++++++++----
 3 files changed, 218 insertions(+), 15 deletions(-)
 create mode 100644 doc/qubes-storage.rst

diff --git a/doc/index.rst b/doc/index.rst
index a9c95a9f..5d0e71d4 100644
--- a/doc/index.rst
+++ b/doc/index.rst
@@ -16,6 +16,7 @@ manpages and API documentation. For primary user documentation, see
    qubes
    qubes-vm/index
    qubes-events
+   qubes-storage
    qubes-exc
    qubes-ext
    qubes-log
diff --git a/doc/qubes-storage.rst b/doc/qubes-storage.rst
new file mode 100644
index 00000000..73366240
--- /dev/null
+++ b/doc/qubes-storage.rst
@@ -0,0 +1,145 @@
+:py:mod:`qubes.storage` -- Qubes data storage
+=============================================
+
+Qubes provide extensible API for domains data storage. Each domain have
+multiple storage volumes, for different purposes. Each volume is provided by
+some storage pool. Qubes support different storage pool drivers, and it's
+possible to register additional 3rd-party drivers.
+
+Domain's storage volumes:
+
+ - `root` - this is where operating system is installed. The volume is
+   available read-write to :py:class:`~qubes.vm.templatevm.TemplateVM` and
+   :py:class:`~qubes.vm.standalonevm.StandaloneVM`, and read-only to others
+   (:py:class:`~qubes.vm.appvm.AppVM` and :py:class:`~qubes.vm.dispvm.DispVM`).
+ - `private` - this is where domain's data live. The volume is available
+   read-write to all domain classes (including :py:class:`~qubes.vm.dispvm.DispVM`,
+   but data written there is discarded on domain shutdown).
+ - `volatile` - this is used for any data that do not to persist. This include
+   swap, copy-on-write layer for `root` volume etc.
+ - `kernel` - domain boot files - operating system kernel, initial ramdisk,
+   kernel modules etc. This volume is provided read-only and should be provided by
+   a storage pool respecting :py:attr:`qubes.vm.qubesvm.QubesVM.kernel` property.
+
+Storage pool concept
+--------------------
+
+Storage pool is responsible for managing its volumes. Qubes have defined
+storage pool driver API, allowing to put domains storage in various places. By
+default two drivers are provided: :py:class:`qubes.storage.file.FilePool`
+(named `file`) and :py:class:`qubes.storage.lvm.ThinPool` (named `lvm_thin`).
+But the API allow to implement variety of other drivers (like additionally
+encrypted storage, external disk, drivers using special features of some
+filesystems like btrfs, etc).
+
+Most of storage API focus on storage volumes. Each volume have at least those
+properties:
+ - :py:attr:`~qubes.storage.Volume.rw` - should the volume be available
+   read-only or read-write to the domain
+ - :py:attr:`~qubes.storage.Volume.snap_on_start` - should the domain start
+   with its own state of the volume, or rather a snapshot of its template volume
+   (pointed by a :py:attr:`~qubes.storage.Volume.source` property). This can be
+   set to `True` only if a domain do have `template` property (AppVM and DispVM).
+   If the domain's template is running already, the snapshot should be made out of
+   the template's before its startup.
+ - :py:attr:`~qubes.storage.Volume.save_on_stop` - should the volume state be
+   saved or discarded on domain
+   stop. In either case, while the domain is running, volume's current state
+   should not be committed immediately. This is to allow creating snapshots of the
+   volume's state from before domain start (see
+   :py:attr:`~qubes.storage.Volume.snap_on_start`).
+ - :py:attr:`~qubes.storage.Volume.revisions_to_keep` - number of volume
+   revisions to keep. If greater than zero, at each domain stop (and if
+   :py:attr:`~qubes.storage.Volume.save_on_stop` is `True`) new revision is saved
+   and old ones exceeding :py:attr:`~qubes.storage.Volume.revisions_to_keep` limit
+   are removed.
+ - :py:attr:`~qubes.storage.Volume.source` - source volume for
+   :py:attr:`~qubes.storage.Volume.snap_on_start` volumes
+ - :py:attr:`~qubes.storage.Volume.vid` - pool specific volume identifier, must
+   be unique inside given pool
+ - :py:attr:`~qubes.storage.Volume.pool` - storage pool object owning this volume
+ - :py:attr:`~qubes.storage.Volume.name` - name of the volume inside owning
+   domain (like `root`, or `private`)
+ - :py:attr:`~qubes.storage.Volume.size` - size of the volume, in bytes
+
+Storage pool driver may define additional properties.
+
+Storage pool driver API
+-----------------------
+
+Storage pool driver need to implement two classes:
+ - pool class - inheriting from :py:class:`qubes.storage.Pool`
+ - volume class - inheriting from :py:class:`qubes.storage.Volume`
+
+Pool class should be registered with `qubes.storage` entry_point, under the
+name of storage pool driver. Volume class instances should be returned by
+:py:meth:`qubes.storage.Pool.init_volume` method of pool class instance.
+
+Methods required to be implemented by the pool class:
+ - :py:meth:`~qubes.storage.Pool.init_volume` - return instance of appropriate
+   volume class; this method should not alter any persistent disk state, it is
+   used to instantiate both existing volumes and create new ones
+ - :py:meth:`~qubes.storage.Pool.setup` - setup new storage pool
+ - :py:meth:`~qubes.storage.Pool.destroy` - destroy storage pool
+
+Methods and properties required to be implemented by the volume class:
+ - :py:meth:`~qubes.storage.Volume.create` - create volume on disk
+ - :py:meth:`~qubes.storage.Volume.remove` - remove volume from disk
+ - :py:meth:`~qubes.storage.Volume.start` - prepare the volume for domain start;
+   this include making a snapshot if
+   :py:attr:`~qubes.storage.Volume.snap_on_start` is `True`
+ - :py:meth:`~qubes.storage.Volume.stop` - cleanup after domain shutdown; this
+   include committing changes to the volume if
+   :py:attr:`~qubes.storage.Volume.save_on_stop` is `True`
+ - :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.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)
+ - :py:meth:`~qubes.storage.Volume.import_data_end` - finish data import
+   operation (cleanup temporary files etc); this methods is called always after
+   :py:meth:`~qubes.storage.Volume.import_data` regardless if operation was
+   successful or not
+ - :py:meth:`~qubes.storage.Volume.import_volume` - import data from another volume
+ - :py:meth:`~qubes.storage.Volume.resize` - resize volume
+ - :py:meth:`~qubes.storage.Volume.revert` - revert volume state to a given revision
+ - :py:attr:`~qubes.storage.Volume.revisions` - collection of volume revisions (to use
+   with :py:meth:`qubes.storage.Volume.revert`)
+ - :py:meth:`~qubes.storage.Volume.is_dirty` - is volume properly committed
+   after domain shutdown? Applies only to volumes with
+   :py:attr:`~qubes.storage.Volume.save_on_stop` set to `True`
+ - :py:meth:`~qubes.storage.Volume.is_outdated` - have the source volume started
+   since domain startup? applies only to volumes with
+   :py:attr:`~qubes.storage.Volume.snap_on_start` set to `True`
+ - :py:attr:`~qubes.storage.Volume.config` - volume configuration, this should
+   be enough to later reinstantiate the same volume object
+ - :py:meth:`~qubes.storage.Volume.block_device` - return
+   :py:class:`qubes.storage.BlockDevice` instance required to configure volume in
+   libvirt
+
+Some storage pool drivers can provide limited functionality only - for example
+support only `volatile` volumes (those with
+:py:attr:`~qubes.storage.Volume.snap_on_start` is `False`,
+:py:attr:`~qubes.storage.Volume.save_on_stop` is `False`, and
+:py:attr:`~qubes.storage.Volume.rw` is `True`). In that case, it should raise
+:py:exc:`NotImplementedError` in :py:meth:`qubes.storage.Pool.init_volume` when
+trying to instantiate unsupported volume.
+
+Note that pool driver should be prepared to recover from power loss before
+stopping a domain - so, if volume have
+:py:attr:`~qubes.storage.Volume.save_on_stop` is `True`, and
+:py:meth:`qubes.storage.Volume.stop` wasn't called, next
+:py:meth:`~qubes.storage.Volume.start` should pick up previous (not committed)
+state.
+
+See specific methods documentation for details.
+
+Module contents
+---------------
+
+.. automodule:: qubes.storage
+   :members:
+   :show-inheritance:
+
+.. vim: ts=3 sw=3 et
diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index 2e7942a8..db3db5f0 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -78,6 +78,8 @@ class Volume(object):
     domain = None
     path = None
     script = None
+    #: disk space used by this volume, can be smaller than :py:attr:`size`
+    #: for sparse volumes
     usage = 0
 
     def __init__(self, name, pool, vid, internal=False, removable=False,
@@ -85,7 +87,7 @@ class Volume(object):
             snap_on_start=False, source=None, **kwargs):
         ''' Initialize a volume.
 
-            :param str name: The domain name
+            :param str name: The name of the volume inside owning domain
             :param Pool pool: The pool object
             :param str vid:  Volume identifier needs to be unique in pool
             :param bool internal: If `True` volume is hidden when qvm-block ls
@@ -94,9 +96,12 @@ class Volume(object):
                 run time
             :param int revisions_to_keep: Amount of revisions to keep around
             :param bool rw: If true volume will be mounted read-write
-            :param bool snap_on_start: Create a snapshot from source on start
-            :param bool save_on_stop: Write changes to disk in vm.stop()
-            :param Volume source: other volume in same pool, or None
+            :param bool snap_on_start: Create a snapshot from source on
+                start, instead of using volume own data
+            :param bool save_on_stop: Write changes to the volume in
+                vm.stop(), otherwise - discard
+            :param Volume source: other volume in same pool to make snapshot
+                from, required if *snap_on_start*=`True`
             :param str/int size: Size of the volume
 
         '''
@@ -106,16 +111,27 @@ class Volume(object):
         assert source is None or (isinstance(source, Volume)
                                   and source.pool == pool)
 
+        #: Name of the volume in a domain it's attached to (like `root` or
+        #: `private`).
         self.name = str(name)
+        #: :py:class:`Pool` instance owning this volume
         self.pool = pool
         self.internal = internal
         self.removable = removable
+        #: How many revisions of the volume to keep. Each revision is created
+        #  at :py:meth:`stop`, if :py:attr:`save_on_stop` is True
         self.revisions_to_keep = int(revisions_to_keep)
+        #: Should this volume be writable by domain.
         self.rw = rw
+        #: Should volume state be saved or discarded at :py:meth:`stop`
         self.save_on_stop = save_on_stop
         self._size = int(size)
+        #: Should the volume state be initialized with a snapshot of
+        #: same-named volume of domain's template.
         self.snap_on_start = snap_on_start
+        #: source volume for :py:attr:`snap_on_start` volumes
         self.source = source
+        #: Volume unique (inside given pool) identifier
         self.vid = vid
 
     def __eq__(self, other):
@@ -142,6 +158,10 @@ class Volume(object):
     def create(self):
         ''' Create the given volume on disk.
 
+            This method is called only once in the volume lifetime. Before
+            calling this method, no data on disk should be touched (in
+            context of this volume).
+
             This can be implemented as a coroutine.
         '''
         raise self._not_implemented("create")
@@ -153,17 +173,37 @@ class Volume(object):
         raise self._not_implemented("remove")
 
     def export(self):
-        ''' Returns an object that can be `open()`. '''
+        ''' Returns a path to read the volume data from.
+
+            Reading from this path when domain owning this volume is
+            running (i.e. when :py:meth:`is_dirty` is True) should return the
+            data from before domain startup.
+
+            Reading from the path returned by this method should return the
+            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.
+        '''
         raise self._not_implemented("export")
 
     def import_data(self):
-        ''' Returns an object that can be `open()`. '''
+        ''' Returns a path to overwrite volume data.
+
+            This method is called after volume was already :py:meth:`create`-ed.
+
+            Writing to this path should overwrite volume data. If importing
+            volume data require something more than just writing to a file (
+            for example connecting to some other domain, or converting data
+            on the fly), the returned path may be a pipe.
+        '''
         raise self._not_implemented("import")
 
     def import_data_end(self, success):
-        ''' End data import operation. This may be used by pool
+        ''' End the data import operation. This may be used by pool
         implementation to commit changes, cleanup temporary files etc.
 
+        This method is called regardless the operation was successful or not.
+
         :param success: True if data import was successful, otherwise False
         '''
         # by default do nothing
@@ -173,19 +213,24 @@ class Volume(object):
         ''' Imports data from a different volume (possibly in a different
         pool.
 
-        The needs to be create()d first.
+        The volume needs to be create()d first.
 
         This can be implemented as a coroutine. '''
         # pylint: disable=unused-argument
         raise self._not_implemented("import_volume")
 
     def is_dirty(self):
-        ''' Return `True` if volume was not properly shutdown and commited '''
+        ''' Return `True` if volume was not properly shutdown and committed.
+
+            This include the situation when domain owning the volume is still
+            running.
+
+        '''
         raise self._not_implemented("is_dirty")
 
     def is_outdated(self):
-        ''' Returns `True` if the currently used `volume.source` of a snapshot
-            volume is outdated.
+        ''' Returns `True` if this snapshot of a source volume (for
+        `snap_on_start`=True) is outdated.
         '''
         raise self._not_implemented("is_outdated")
 
@@ -195,23 +240,33 @@ class Volume(object):
             given size is less than current_size
 
             This can be implemented as a coroutine.
+
+            :param int size: new size in bytes
         '''
         # pylint: disable=unused-argument
         raise self._not_implemented("resize")
 
     def revert(self, revision=None):
-        ''' Revert volume to previous revision  '''
+        ''' Revert volume to previous revision
+
+        :param revision: revision to revert volume to, see :py:attr:`revisions`
+        '''
         # pylint: disable=unused-argument
         raise self._not_implemented("revert")
 
     def start(self):
-        ''' Do what ever is needed on start
+        ''' Do what ever is needed on start.
+
+        This include making a snapshot of template's volume if
+        :py:attr:`snap_on_start` is set.
 
         This can be implemented as a coroutine.'''
         raise self._not_implemented("start")
 
     def stop(self):
-        ''' Do what ever is needed on stop
+        ''' Do what ever is needed on stop.
+
+        This include committing data if :py:attr:`save_on_stop` is set.
 
         This can be implemented as a coroutine.'''
 
@@ -230,12 +285,14 @@ class Volume(object):
 
     @property
     def revisions(self):
-        ''' Returns a `dict` containing revision identifiers and paths '''
+        ''' Returns a dict containing revision identifiers and time of their
+        creation '''
         msg = "{!s} has revisions not implemented".format(self.__class__)
         raise NotImplementedError(msg)
 
     @property
     def size(self):
+        ''' Volume size in bytes '''
         return self._size
 
     @size.setter

From 1a1dd3dba258f6ceffd69dad123cb018ecde6a5b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 1 Jul 2017 21:29:47 +0200
Subject: [PATCH 06/12] storage: make default pool configurable

Do not always use pool named 'default'. Instead, have global
`default_pool` property to specify default storage pools.
Additionally add `default_pool_*` properties for each VM property, so
those can be set separately.

QubesOS/qubes-issues#2256
---
 qubes/app.py              | 64 +++++++++++++++++++++++++++++++++++++++
 qubes/storage/__init__.py |  5 ++-
 2 files changed, 68 insertions(+), 1 deletion(-)

diff --git a/qubes/app.py b/qubes/app.py
index 493bc412..347c30a2 100644
--- a/qubes/app.py
+++ b/qubes/app.py
@@ -28,6 +28,7 @@ import grp
 import logging
 import os
 import random
+import subprocess
 import sys
 import tempfile
 import time
@@ -548,6 +549,46 @@ class VMCollection(object):
             'https://xkcd.com/221/',
             'http://dilbert.com/strip/2001-10-25')[random.randint(0, 1)])
 
+def _default_pool(app):
+    ''' Default storage pool.
+
+    1. If there is one named 'default', use it.
+    2. Check if root fs is on LVM thin - use that
+    3. Look for file-based pool pointing /var/lib/qubes
+    4. Fail
+    '''
+    if 'default' in app.pools:
+        return app.pools['default']
+    else:
+        rootfs = os.stat('/')
+        root_major = (rootfs.st_dev & 0xff00) >> 8
+        root_minor = rootfs.st_dev & 0xff
+        for pool in app.pools.values():
+            if pool.config.get('driver', None) != 'lvm_thin':
+                continue
+            thin_pool = pool.config['thin_pool']
+            thin_volumes = subprocess.check_output(
+                ['lvs', '--select', 'pool_lv=' + thin_pool,
+                '-o', 'lv_kernel_major,lv_kernel_minor', '--noheadings'])
+            if any((str(root_major), str(root_minor)) == thin_vol.split()
+                    for thin_vol in thin_volumes.splitlines()):
+                return pool
+        # not a thin volume? look for file pools
+        for pool in app.pools.values():
+            if pool.config.get('driver', None) != 'file':
+                continue
+            if pool.config['dir_path'] == '/var/lib/qubes':
+                return pool
+        raise AttributeError('Cannot determine default storage pool')
+
+def _setter_pool(app, prop, value):
+    if isinstance(value, qubes.storage.Pool):
+        return value
+    try:
+        return app.pools[value]
+    except KeyError:
+        raise qubes.exc.QubesPropertyValueError(app, prop, value,
+            'No such storage pool')
 
 class Qubes(qubes.PropertyHolder):
     '''Main Qubes application
@@ -629,6 +670,27 @@ class Qubes(qubes.PropertyHolder):
     default_dispvm = qubes.VMProperty('default_dispvm', load_stage=3,
         doc='Default DispVM base for service calls')
 
+    default_pool = qubes.property('default_pool', load_stage=3,
+        default=_default_pool,
+        doc='Default storage pool')
+
+    default_pool_private = qubes.property('default_pool_private', load_stage=3,
+        default=lambda app: app.default_pool,
+        doc='Default storage pool for private volumes')
+
+    default_pool_root = qubes.property('default_pool_root', load_stage=3,
+        default=lambda app: app.default_pool,
+        doc='Default storage pool for root volumes')
+
+    default_pool_volatile = qubes.property('default_pool_volatile',
+        load_stage=3,
+        default=lambda app: app.default_pool,
+        doc='Default storage pool for volatile volumes')
+
+    default_pool_kernel = qubes.property('default_pool_kernel', load_stage=3,
+        default=lambda app: app.default_pool,
+        doc='Default storage pool for kernel volumes')
+
     # TODO #1637 #892
     check_updates_vm = qubes.property('check_updates_vm',
         type=bool, setter=qubes.property.bool,
@@ -907,6 +969,8 @@ class Qubes(qubes.PropertyHolder):
         for name, config in qubes.config.defaults['pool_configs'].items():
             self.pools[name] = self._get_pool(**config)
 
+        self.default_pool_kernel = 'linux-kernel'
+
         self.domains.add(
             qubes.vm.adminvm.AdminVM(self, None, label='black'))
 
diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index db3db5f0..dd453498 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -369,7 +369,10 @@ class Storage(object):
 
         if 'name' not in volume_config:
             volume_config['name'] = name
-        pool = self.vm.app.get_pool(volume_config['pool'])
+        if 'pool' not in volume_config:
+            pool = getattr(self.vm.app, 'default_pool_' + name)
+        else:
+            pool = self.vm.app.get_pool(volume_config['pool'])
         volume = pool.init_volume(self.vm, volume_config)
         self.vm.volumes[name] = volume
         return volume

From 317d140f46d8580ca3f6259d5d9d4ed67a4aa08f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 1 Jul 2017 22:45:22 +0200
Subject: [PATCH 07/12] 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   |   9 ++
 qubes/storage/file.py       | 197 +++++++++++++-----------------------
 qubes/tests/storage_file.py |  33 +++++-
 3 files changed, 114 insertions(+), 125 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index dd453498..ff135af2 100644
--- a/qubes/storage/__init__.py
+++ b/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)
diff --git a/qubes/storage/file.py b/qubes/storage/file.py
index f683114e..84013f4a 100644
--- a/qubes/storage/file.py
+++ b/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:
-            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:
+        if self.snap_on_start:
             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'
-        if self._is_origin:
+            'Volume size must be > 0'
+        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:
-            return  # do not remove random attached file volumes
-        elif self._is_snapshot:
-            return  # no need to remove, because it's just a snapshot
-        else:
+        if not self.snap_on_start:
             _remove_if_exists(self.path)
-            if self._is_origin:
-                _remove_if_exists(self.path_cow)
+        if self.snap_on_start or self.save_on_stop:
+            _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
-
-        if self._is_volume:
-            return self
+        assert self.save_on_stop and self.rw, msg
 
         if os.path.exists(self.path_cow):
-            old_path = self.path_cow + '.old'
-            os.rename(self.path_cow, old_path)
+            if self.revisions_to_keep:
+                old_path = self.path_cow + '.old'
+                os.rename(self.path_cow, old_path)
+            else:
+                os.unlink(self.path_cow)
 
-        old_umask = os.umask(0o002)
-        with open(self.path_cow, 'w') as f_cow:
-            f_cow.truncate(self.size)
-        os.umask(old_umask)
+        create_sparse_file(self.path_cow, self.size)
         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)
-
-        with open(self.path, "w") as f_volatile:
-            f_volatile.truncate(self.size)
+        create_sparse_file(self.path, 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 self.snap_on_start:
-                if not self.save_on_stop:
-                    # make sure previous snapshot is removed - even if VM
-                    # shutdown routing wasn't called (power interrupt or so)
-                    _remove_if_exists(self.path_cow)
-                try:
-                    _check_path(self.path_cow)
-                except qubes.storage.StoragePoolException:
-                    create_sparse_file(self.path_cow, self.size)
-                    _check_path(self.path_cow)
-                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)
+            if not self.save_on_stop:
+                # make sure previous snapshot is removed - even if VM
+                # shutdown routine wasn't called (power interrupt or so)
+                _remove_if_exists(self.path_cow)
+            if not os.path.exists(self.path_cow):
+                create_sparse_file(self.path_cow, self.size)
+            if not self.snap_on_start:
+                _check_path(self.path)
+            if hasattr(self, 'path_source_cow'):
+                if not os.path.exists(self.path_source_cow):
+                    create_sparse_file(self.path_source_cow, self.size)
         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 '''
diff --git a/qubes/tests/storage_file.py b/qubes/tests/storage_file.py
index 4483b55b..4843d1b8 100644
--- a/qubes/tests/storage_file.py
+++ b/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)

From 075d9911141d4bd52f4c24df213fb147ad111bb7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sat, 1 Jul 2017 23:25:47 +0200
Subject: [PATCH 08/12] config: eliminate duplicated qubes_base_dir

Remove it from system_path dict, have it in one place.
---
 qubes/app.py                | 2 +-
 qubes/config.py             | 2 --
 qubes/tests/__init__.py     | 6 +++---
 qubes/tests/storage_file.py | 6 +++---
 qubes/vm/qubesvm.py         | 4 ++--
 5 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/qubes/app.py b/qubes/app.py
index 347c30a2..4005836d 100644
--- a/qubes/app.py
+++ b/qubes/app.py
@@ -724,7 +724,7 @@ class Qubes(qubes.PropertyHolder):
         else:
             self._store = os.environ.get('QUBES_XML_PATH',
                 os.path.join(
-                    qubes.config.system_path['qubes_base_dir'],
+                    qubes.config.qubes_base_dir,
                     qubes.config.system_path['qubes_store_filename']))
 
         super(Qubes, self).__init__(xml=None, **kwargs)
diff --git a/qubes/config.py b/qubes/config.py
index 5ef2e047..8bc3864e 100644
--- a/qubes/config.py
+++ b/qubes/config.py
@@ -36,8 +36,6 @@ system_path = {
     'qrexec_client_path': '/usr/lib/qubes/qrexec-client',
     'qubesdb_daemon_path': '/usr/sbin/qubesdb-daemon',
 
-    'qubes_base_dir': qubes_base_dir,
-
     # Relative to qubes_base_dir
     'qubes_appvms_dir': 'appvms',
     'qubes_templates_dir': 'vm-templates',
diff --git a/qubes/tests/__init__.py b/qubes/tests/__init__.py
index 3d19249e..64244185 100644
--- a/qubes/tests/__init__.py
+++ b/qubes/tests/__init__.py
@@ -583,7 +583,7 @@ class SystemTestsMixin(object):
         # need some information from the real qubes.xml - at least installed
         # templates; should not be used for testing, only to initialize self.app
         self.host_app = qubes.Qubes(os.path.join(
-            qubes.config.system_path['qubes_base_dir'],
+            qubes.config.qubes_base_dir,
             qubes.config.system_path['qubes_store_filename']))
         if os.path.exists(CLASS_XMLPATH):
             shutil.copy(CLASS_XMLPATH, XMLPATH)
@@ -727,7 +727,7 @@ class SystemTestsMixin(object):
                 'qubes_appvms_dir',
                 'qubes_servicevms_dir',
                 'qubes_templates_dir'):
-            dirpath = os.path.join(qubes.config.system_path['qubes_base_dir'],
+            dirpath = os.path.join(qubes.config.qubes_base_dir,
                 qubes.config.system_path[dirspec], vmname)
             if os.path.exists(dirpath):
                 if os.path.isdir(dirpath):
@@ -791,7 +791,7 @@ class SystemTestsMixin(object):
                 'qubes_appvms_dir',
                 'qubes_servicevms_dir',
                 'qubes_templates_dir'):
-            dirpath = os.path.join(qubes.config.system_path['qubes_base_dir'],
+            dirpath = os.path.join(qubes.config.qubes_base_dir,
                 qubes.config.system_path[dirspec])
             for name in os.listdir(dirpath):
                 if name.startswith(prefix):
diff --git a/qubes/tests/storage_file.py b/qubes/tests/storage_file.py
index 4843d1b8..f2b80835 100644
--- a/qubes/tests/storage_file.py
+++ b/qubes/tests/storage_file.py
@@ -306,8 +306,8 @@ class TC_03_FilePool(qubes.tests.QubesTestCase):
     def setUp(self):
         """ Add a test file based storage pool """
         super(TC_03_FilePool, self).setUp()
-        self._orig_qubes_base_dir = qubes.config.system_path['qubes_base_dir']
-        qubes.config.system_path['qubes_base_dir'] = '/tmp/qubes-test'
+        self._orig_qubes_base_dir = qubes.config.qubes_base_dir
+        qubes.config.qubes_base_dir = '/tmp/qubes-test'
         self.app = TestApp()
         self.app.create_dummy_template()
         self.app.add_pool(**self.POOL_CONFIG)
@@ -320,7 +320,7 @@ class TC_03_FilePool(qubes.tests.QubesTestCase):
         shutil.rmtree(self.POOL_DIR, ignore_errors=True)
         if os.path.exists('/tmp/qubes-test'):
             shutil.rmtree('/tmp/qubes-test')
-        qubes.config.system_path['qubes_base_dir'] = self._orig_qubes_base_dir
+        qubes.config.qubes_base_dir = self._orig_qubes_base_dir
 
     def test_001_pool_exists(self):
         """ Check if the storage pool was added to the storage pool config """
diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index 2c0ea441..0f2f0530 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -577,7 +577,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
     def dir_path(self):
         '''Root directory for files related to this domain'''
         return os.path.join(
-            qubes.config.system_path['qubes_base_dir'],
+            qubes.config.qubes_base_dir,
             self.dir_path_prefix,
             self.name)
 
@@ -716,7 +716,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
         if not newvalue:
             return
         dirname = os.path.join(
-            qubes.config.system_path['qubes_base_dir'],
+            qubes.config.qubes_base_dir,
             qubes.config.system_path['qubes_kernels_base_dir'],
             newvalue)
         if not os.path.exists(dirname):

From 0e554296e37f7bf0519975f6bd2ab9b4906ff748 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Sun, 2 Jul 2017 01:07:48 +0200
Subject: [PATCH 09/12] storage: drop 'internal' and 'removable' volume
 properties

Since dynamic volumes (qvm-block) are moved to devices API, those two
are not needed anymore.

QubesOS/qubes-issues#2256
---
 qubes/api/admin.py           |  2 +-
 qubes/storage/__init__.py    | 13 ++++---------
 qubes/storage/kernels.py     |  1 -
 qubes/tests/api_admin.py     |  2 +-
 qubes/tests/integ/storage.py |  7 -------
 qubes/vm/appvm.py            |  4 ----
 qubes/vm/dispvm.py           |  4 ----
 qubes/vm/qubesvm.py          |  6 +++---
 qubes/vm/standalonevm.py     |  4 ----
 qubes/vm/templatevm.py       |  4 ----
 10 files changed, 9 insertions(+), 38 deletions(-)

diff --git a/qubes/api/admin.py b/qubes/api/admin.py
index 5a72c1e2..de5b8ccb 100644
--- a/qubes/api/admin.py
+++ b/qubes/api/admin.py
@@ -261,7 +261,7 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
         volume = self.dest.volumes[self.arg]
         # properties defined in API
         volume_properties = [
-            'pool', 'vid', 'size', 'usage', 'rw', 'internal', 'source',
+            'pool', 'vid', 'size', 'usage', 'rw', 'source',
             'save_on_stop', 'snap_on_start']
         return ''.join('{}={}\n'.format(key, getattr(volume, key)) for key in
             volume_properties)
diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index ff135af2..63ce351b 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -82,7 +82,7 @@ class Volume(object):
     #: for sparse volumes
     usage = 0
 
-    def __init__(self, name, pool, vid, internal=False, removable=False,
+    def __init__(self, name, pool, vid,
             revisions_to_keep=0, rw=False, save_on_stop=False, size=0,
             snap_on_start=False, source=None, **kwargs):
         ''' Initialize a volume.
@@ -90,10 +90,6 @@ class Volume(object):
             :param str name: The name of the volume inside owning domain
             :param Pool pool: The pool object
             :param str vid:  Volume identifier needs to be unique in pool
-            :param bool internal: If `True` volume is hidden when qvm-block ls
-                is used
-            :param bool removable: If `True` volume can be detached from vm at
-                run time
             :param int revisions_to_keep: Amount of revisions to keep around
             :param bool rw: If true volume will be mounted read-write
             :param bool snap_on_start: Create a snapshot from source on
@@ -125,8 +121,6 @@ class Volume(object):
         self.name = str(name)
         #: :py:class:`Pool` instance owning this volume
         self.pool = pool
-        self.internal = internal
-        self.removable = removable
         #: How many revisions of the volume to keep. Each revision is created
         #  at :py:meth:`stop`, if :py:attr:`save_on_stop` is True
         self.revisions_to_keep = int(revisions_to_keep)
@@ -317,8 +311,6 @@ class Volume(object):
             'name': self.name,
             'pool': str(self.pool),
             'vid': self.vid,
-            'internal': self.internal,
-            'removable': self.removable,
             'revisions_to_keep': self.revisions_to_keep,
             'rw': self.rw,
             'save_on_stop': self.save_on_stop,
@@ -382,6 +374,9 @@ class Storage(object):
             pool = getattr(self.vm.app, 'default_pool_' + name)
         else:
             pool = self.vm.app.get_pool(volume_config['pool'])
+        if 'internal' in volume_config:
+            # migrate old config
+            del volume_config['internal']
         volume = pool.init_volume(self.vm, volume_config)
         self.vm.volumes[name] = volume
         return volume
diff --git a/qubes/storage/kernels.py b/qubes/storage/kernels.py
index 60d304eb..1baf7c78 100644
--- a/qubes/storage/kernels.py
+++ b/qubes/storage/kernels.py
@@ -167,7 +167,6 @@ class LinuxKernel(Pool):
                              kernel_version,
                              pool=self,
                              name=kernel_version,
-                             internal=True,
                              rw=False
                              )
                 for kernel_version in os.listdir(self.dir_path)]
diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py
index 8b6eadcf..6216d98a 100644
--- a/qubes/tests/api_admin.py
+++ b/qubes/tests/api_admin.py
@@ -37,7 +37,7 @@ import qubes.storage
 
 # properties defined in API
 volume_properties = [
-    'pool', 'vid', 'size', 'usage', 'rw', 'internal', 'source',
+    'pool', 'vid', 'size', 'usage', 'rw', 'source',
     'save_on_stop', 'snap_on_start']
 
 
diff --git a/qubes/tests/integ/storage.py b/qubes/tests/integ/storage.py
index 8f93d9da..401ef08e 100644
--- a/qubes/tests/integ/storage.py
+++ b/qubes/tests/integ/storage.py
@@ -59,7 +59,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'save_on_stop': False,
             'rw': True,
         }
@@ -91,7 +90,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'save_on_stop': True,
             'rw': True,
         }
@@ -126,7 +124,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'save_on_stop': False,
             'rw': False,
         }
@@ -157,7 +154,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'save_on_stop': True,
             'rw': True,
         }
@@ -166,7 +162,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'snap_on_start': True,
             'source': testvol.vid,
             'rw': True,
@@ -225,7 +220,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'save_on_stop': True,
             'rw': True,
         }
@@ -234,7 +228,6 @@ class StorageTestMixin(qubes.tests.SystemTestsMixin):
         volume_config = {
             'pool': self.pool.name,
             'size': size,
-            'internal': False,
             'snap_on_start': True,
             'source': testvol.vid,
             'rw': True,
diff --git a/qubes/vm/appvm.py b/qubes/vm/appvm.py
index b818c995..8d22e5db 100644
--- a/qubes/vm/appvm.py
+++ b/qubes/vm/appvm.py
@@ -49,7 +49,6 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
                 'save_on_stop': False,
                 'rw': False,
                 'source': None,
-                'internal': True
             },
             'private': {
                 'name': 'private',
@@ -58,7 +57,6 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
                 'save_on_stop': True,
                 'rw': True,
                 'size': defaults['private_img_size'],
-                'internal': True
             },
             'volatile': {
                 'name': 'volatile',
@@ -66,7 +64,6 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
                 'snap_on_start': False,
                 'save_on_stop': False,
                 'size': defaults['root_img_size'],
-                'internal': True,
                 'rw': True,
             },
             'kernel': {
@@ -75,7 +72,6 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
                 'snap_on_start': False,
                 'save_on_stop': False,
                 'rw': False,
-                'internal': True
             }
         }
 
diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py
index 8586627b..97d67d49 100644
--- a/qubes/vm/dispvm.py
+++ b/qubes/vm/dispvm.py
@@ -47,20 +47,17 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
                 'snap_on_start': True,
                 'save_on_stop': False,
                 'rw': False,
-                'internal': True
             },
             'private': {
                 'name': 'private',
                 'pool': 'default',
                 'snap_on_start': True,
                 'save_on_stop': False,
-                'internal': True,
                 'rw': True,
             },
             'volatile': {
                 'name': 'volatile',
                 'pool': 'default',
-                'internal': True,
                 'snap_on_start': False,
                 'save_on_stop': False,
                 'rw': True,
@@ -73,7 +70,6 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
                 'snap_on_start': False,
                 'save_on_stop': False,
                 'rw': False,
-                'internal': True
             }
         }
         if 'name' not in kwargs and 'dispid' in kwargs:
diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index 0f2f0530..9b1261af 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -1815,9 +1815,9 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
 
 
 def _clean_volume_config(config):
-    common_attributes = ['name', 'pool', 'size', 'internal', 'removable',
-                            'revisions_to_keep', 'rw', 'snap_on_start',
-                            'save_on_stop', 'source']
+    common_attributes = ['name', 'pool', 'size',
+                         'revisions_to_keep', 'rw', 'snap_on_start',
+                         'save_on_stop', 'source']
     config_copy = copy.deepcopy(config)
     return {k: v for k, v in config_copy.items() if k in common_attributes}
 
diff --git a/qubes/vm/standalonevm.py b/qubes/vm/standalonevm.py
index 7aab434d..582ad65e 100644
--- a/qubes/vm/standalonevm.py
+++ b/qubes/vm/standalonevm.py
@@ -35,7 +35,6 @@ class StandaloneVM(qubes.vm.qubesvm.QubesVM):
                 'save_on_stop': True,
                 'rw': True,
                 'source': None,
-                'internal': True,
                 'size': qubes.config.defaults['root_img_size'],
             },
             'private': {
@@ -45,7 +44,6 @@ class StandaloneVM(qubes.vm.qubesvm.QubesVM):
                 'save_on_stop': True,
                 'rw': True,
                 'source': None,
-                'internal': True,
                 'size': qubes.config.defaults['private_img_size'],
             },
             'volatile': {
@@ -53,7 +51,6 @@ class StandaloneVM(qubes.vm.qubesvm.QubesVM):
                 'pool': 'default',
                 'snap_on_start': False,
                 'save_on_stop': False,
-                'internal': True,
                 'rw': True,
                 'size': qubes.config.defaults['root_img_size'],
             },
@@ -63,7 +60,6 @@ class StandaloneVM(qubes.vm.qubesvm.QubesVM):
                 'snap_on_start': False,
                 'save_on_stop': False,
                 'rw': False,
-                'internal': True
             }
         }
         super(StandaloneVM, self).__init__(*args, **kwargs)
diff --git a/qubes/vm/templatevm.py b/qubes/vm/templatevm.py
index 8cd07528..89128ba3 100644
--- a/qubes/vm/templatevm.py
+++ b/qubes/vm/templatevm.py
@@ -71,7 +71,6 @@ class TemplateVM(QubesVM):
                 'rw': True,
                 'source': None,
                 'size': defaults['root_img_size'],
-                'internal': True
             },
             'private': {
                 'name': 'private',
@@ -82,7 +81,6 @@ class TemplateVM(QubesVM):
                 'source': None,
                 'size': defaults['private_img_size'],
                 'revisions_to_keep': 0,
-                'internal': True
             },
             'volatile': {
                 'name': 'volatile',
@@ -90,7 +88,6 @@ class TemplateVM(QubesVM):
                 'size': defaults['root_img_size'],
                 'snap_on_start': False,
                 'save_on_stop': False,
-                'internal': True,
                 'rw': True,
             },
             'kernel': {
@@ -98,7 +95,6 @@ class TemplateVM(QubesVM):
                 'pool': 'linux-kernel',
                 'snap_on_start': False,
                 'save_on_stop': False,
-                'internal': True,
                 'rw': False
             }
         }

From c7ca4a445ecab20ed1e5d6999d1414ab71f28985 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 3 Jul 2017 23:39:13 +0200
Subject: [PATCH 10/12] storage/kernels: support only save_on_stop=False
 volumes

LinuxKernel pool support only read-only volumes, so save_on_stop=True
doesn't make sense. Make it more explicit - raise NotImplementedError
otherwise.
Also, migrate old configs where snap_on_start=True, but no source was
given.

QubesOS/qubes-issues#2256
---
 qubes/storage/kernels.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qubes/storage/kernels.py b/qubes/storage/kernels.py
index 1baf7c78..b570d38c 100644
--- a/qubes/storage/kernels.py
+++ b/qubes/storage/kernels.py
@@ -138,6 +138,14 @@ class LinuxKernel(Pool):
     def init_volume(self, vm, volume_config):
         assert not volume_config['rw']
 
+        # migrate old config
+        if volume_config.get('snap_on_start', False) and not \
+                volume_config.get('source', None):
+            volume_config['snap_on_start'] = False
+
+        if volume_config.get('save_on_stop', False):
+            raise NotImplementedError(
+                'LinuxKernel pool does not support save_on_stop=True')
         volume_config['pool'] = self
         volume = LinuxModules(self.dir_path, lambda: vm.kernel, **volume_config)
 

From 12adf8bede57dc66848d6b7b0453d17912739993 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Tue, 4 Jul 2017 03:29:54 +0200
Subject: [PATCH 11/12] storage/lvm: major cleanup, update

- remove obsolete volume types, use snap_on_start/save_on_stop directly
- handle multiple revisions
- implement is_outdated()

QubesOS/qubes-issues#2256
---
 qubes/storage/lvm.py | 134 ++++++++++++++++++++++++++-----------------
 1 file changed, 81 insertions(+), 53 deletions(-)

diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py
index 751daf60..09baa286 100644
--- a/qubes/storage/lvm.py
+++ b/qubes/storage/lvm.py
@@ -21,9 +21,14 @@
 ''' Driver for storing vm images in a LVM thin pool '''
 
 import logging
+import operator
 import os
 import subprocess
 
+import time
+
+import asyncio
+
 import qubes
 import qubes.storage
 import qubes.utils
@@ -102,6 +107,9 @@ class ThinPool(qubes.storage.Pool):
             if vid.endswith('-snap'):
                 # implementation detail volume
                 continue
+            if vid.endswith('-back'):
+                # old revisions
+                continue
             config = {
                 'pool': self,
                 'vid': vid,
@@ -115,7 +123,7 @@ class ThinPool(qubes.storage.Pool):
 
 def init_cache(log=logging.getLogger('qube.storage.lvm')):
     cmd = ['lvs', '--noheadings', '-o',
-           'vg_name,pool_lv,name,lv_size,data_percent,lv_attr',
+           'vg_name,pool_lv,name,lv_size,data_percent,lv_attr,origin',
            '--units', 'b', '--separator', ',']
     if os.getuid() != 0:
         cmd.insert(0, 'sudo')
@@ -132,14 +140,15 @@ def init_cache(log=logging.getLogger('qube.storage.lvm')):
 
     for line in out.splitlines():
         line = line.decode().strip()
-        pool_name, pool_lv, name, size, usage_percent, attr = line.split(',', 5)
+        pool_name, pool_lv, name, size, usage_percent, attr, \
+            origin = line.split(',', 6)
         if '' in [pool_name, pool_lv, name, size, usage_percent]:
             continue
         name = pool_name + "/" + name
         size = int(size[:-1])
         usage = int(size / 100 * float(usage_percent))
         result[name] = {'size': size, 'usage': usage, 'pool_lv': pool_lv,
-            'attr': attr}
+            'attr': attr, 'origin': origin}
 
     return result
 
@@ -156,16 +165,7 @@ class ThinVolume(qubes.storage.Volume):
         super(ThinVolume, self).__init__(size=size, **kwargs)
         self.log = logging.getLogger('qube.storage.lvm.%s' % str(self.pool))
 
-        if self.snap_on_start and self.source is None:
-            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.snap_on_start:
+        if self.snap_on_start or self.save_on_stop:
             self._vid_snap = self.vid + '-snap'
 
         self._size = size
@@ -176,28 +176,18 @@ class ThinVolume(qubes.storage.Volume):
 
     @property
     def revisions(self):
-        path = self.path + '-back'
-        if os.path.exists(path):
-            seconds = os.path.getctime(path)
+        name_prefix = self.vid + '-'
+        revisions = {}
+        for revision_vid in size_cache:
+            if not revision_vid.startswith(name_prefix):
+                continue
+            if not revision_vid.endswith('-back'):
+                continue
+            revision_vid = revision_vid[len(name_prefix):]
+            seconds = int(revision_vid[:-len('-back')])
             iso_date = qubes.storage.isodate(seconds).split('.', 1)[0]
-            return {iso_date: path}
-        return {}
-
-    @property
-    def _is_origin(self):
-        return not self.snap_on_start and self.save_on_stop
-
-    @property
-    def _is_origin_snapshot(self):
-        return self.snap_on_start and self.save_on_stop
-
-    @property
-    def _is_snapshot(self):
-        return self.snap_on_start and not self.save_on_stop
-
-    @property
-    def _is_volatile(self):
-        return not self.snap_on_start and not self.save_on_stop
+            revisions[revision_vid] = iso_date
+        return revisions
 
     @property
     def size(self):
@@ -213,8 +203,8 @@ class ThinVolume(qubes.storage.Volume):
 
     def _reset(self):
         ''' Resets a volatile volume '''
-        assert self._is_volatile, \
-            'Expected a volatile volume, but got {!r}'.format(self)
+        assert not self.snap_on_start and not self.save_on_stop, \
+            "Not a volatile volume"
         self.log.debug('Resetting volatile ' + self.vid)
         try:
             cmd = ['remove', self.vid]
@@ -226,6 +216,27 @@ class ThinVolume(qubes.storage.Volume):
                str(self.size)]
         qubes_lvm(cmd, self.log)
 
+    def _remove_revisions(self, revisions=None):
+        '''Remove old volume revisions.
+
+        If no revisions list is given, it removes old revisions according to
+        :py:attr:`revisions_to_keep`
+
+        :param revisions: list of revisions to remove
+        '''
+        if revisions is None:
+            revisions = sorted(self.revisions.items(),
+                key=operator.itemgetter(1))
+            revisions = revisions[:-self.revisions_to_keep]
+            revisions = [rev_id for rev_id, _ in revisions]
+
+        for rev_id in revisions:
+            try:
+                cmd = ['remove', self.vid + rev_id]
+                qubes_lvm(cmd, self.log)
+            except qubes.storage.StoragePoolException:
+                pass
+
     def _commit(self):
         msg = "Trying to commit {!s}, but it has save_on_stop == False"
         msg = msg.format(self)
@@ -236,13 +247,11 @@ class ThinVolume(qubes.storage.Volume):
         assert self.rw, msg
         assert hasattr(self, '_vid_snap')
 
-        try:
-            cmd = ['remove', self.vid + "-back"]
+        if self.revisions_to_keep > 0:
+            cmd = ['clone', self.vid,
+                '{}-{}-back'.format(self.vid, int(time.time()))]
             qubes_lvm(cmd, self.log)
-        except qubes.storage.StoragePoolException:
-            pass
-        cmd = ['clone', self.vid, self.vid + "-back"]
-        qubes_lvm(cmd, self.log)
+            self._remove_revisions()
 
         cmd = ['remove', self.vid]
         qubes_lvm(cmd, self.log)
@@ -273,6 +282,7 @@ class ThinVolume(qubes.storage.Volume):
             cmd = ['remove', self._vid_snap]
             qubes_lvm(cmd, self.log)
 
+        self._remove_revisions(self.revisions.keys())
         if not os.path.exists(self.path):
             return
         cmd = ['remove', self.vid]
@@ -284,6 +294,7 @@ class ThinVolume(qubes.storage.Volume):
         devpath = '/dev/' + self.vid
         return devpath
 
+    @asyncio.coroutine
     def import_volume(self, src_volume):
         if not src_volume.save_on_stop:
             return self
@@ -299,9 +310,14 @@ class ThinVolume(qubes.storage.Volume):
             qubes_lvm(cmd, self.log)
         else:
             src_path = src_volume.export()
-            cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + self.vid,
+            cmd = ['dd', 'if=' + src_path, 'of=/dev/' + self.vid,
                 'conv=sparse']
-            subprocess.check_call(cmd)
+            p = yield from asyncio.create_subprocess_exec(*cmd)
+            yield from p.wait()
+            if p.returncode != 0:
+                raise qubes.storage.StoragePoolException(
+                    'Failed to import volume {!r}, dd exit code: {}'.format(
+                        src_volume, p.returncode))
             reset_cache()
 
         return self
@@ -313,18 +329,30 @@ class ThinVolume(qubes.storage.Volume):
 
     def is_dirty(self):
         if self.save_on_stop:
-            return os.path.exists(self.path + '-snap')
+            return os.path.exists('/dev/' + self._vid_snap)
         return False
 
+    def is_outdated(self):
+        if not self.snap_on_start:
+            return False
+        if self._vid_snap not in size_cache:
+            return False
+        return (size_cache[self._vid_snap]['origin'] !=
+               self.source.vid.split('/')[1])
+
+
     def revert(self, revision=None):
-        old_path = self.path + '-back'
+        if revision is None:
+            revision = \
+                max(self.revisions.items(), key=operator.itemgetter(1))[0]
+        old_path = self.path + '-' + revision
         if not os.path.exists(old_path):
             msg = "Volume {!s} has no {!s}".format(self, old_path)
             raise qubes.storage.StoragePoolException(msg)
 
         cmd = ['remove', self.vid]
         qubes_lvm(cmd, self.log)
-        cmd = ['clone', self.vid + '-back', self.vid]
+        cmd = ['clone', self.vid + '-' + revision, self.vid]
         qubes_lvm(cmd, self.log)
         reset_cache()
         return self
@@ -364,22 +392,22 @@ class ThinVolume(qubes.storage.Volume):
 
 
     def start(self):
-        if self.snap_on_start:
+        if self.snap_on_start or self.save_on_stop:
             if not self.save_on_stop or not self.is_dirty():
                 self._snapshot()
-        elif not self.save_on_stop:
+        else:
             self._reset()
 
         reset_cache()
         return self
 
     def stop(self):
-        if self.save_on_stop and self.snap_on_start:
+        if self.save_on_stop:
             self._commit()
-        if self.snap_on_start:
+        if self.snap_on_start or self.save_on_stop:
             cmd = ['remove', self._vid_snap]
             qubes_lvm(cmd, self.log)
-        elif not self.save_on_stop:
+        else:
             cmd = ['remove', self.vid]
             qubes_lvm(cmd, self.log)
         reset_cache()
@@ -398,7 +426,7 @@ class ThinVolume(qubes.storage.Volume):
         ''' Return :py:class:`qubes.storage.BlockDevice` for serialization in
             the libvirt XML template as <disk>.
         '''
-        if self.snap_on_start:
+        if self.snap_on_start or self.save_on_stop:
             return qubes.storage.BlockDevice(
                 '/dev/' + self._vid_snap, self.name, self.script,
                 self.rw, self.domain, self.devtype)

From cfbccc0a749ecb223fb1b76a98c008738a1f37a3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Tue, 4 Jul 2017 03:32:32 +0200
Subject: [PATCH 12/12] vm: drop old is_outdated/commit_changes

This code is unused now. Theoretically this is_outdated implementation
should be moved to FileVolume, but since we don't have VM reference
there, it isn't possible to read appropriate xenstore entry. As we're
phasing out file pool, simply ignore it for now.

QubesOS/qubes-issues#2256
---
 qubes/vm/qubesvm.py    | 39 ---------------------------------------
 qubes/vm/templatevm.py | 10 ----------
 2 files changed, 49 deletions(-)

diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py
index 9b1261af..2fe480a1 100644
--- a/qubes/vm/qubesvm.py
+++ b/qubes/vm/qubesvm.py
@@ -1643,45 +1643,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
 
         return None
 
-    def is_outdated(self):
-        '''Check whether domain needs restart to update root image from \
-            template.
-
-        :returns: :py:obj:`True` if is outdated, :py:obj:`False` otherwise.
-        :rtype: bool
-        '''
-        # pylint: disable=no-member
-
-        # Makes sense only on VM based on template
-        if self.template is None:
-            return False
-
-        if not self.is_running():
-            return False
-
-        if not hasattr(self.template, 'rootcow_img'):
-            return False
-
-        rootimg_inode = os.stat(self.template.root_img)
-        try:
-            rootcow_inode = os.stat(self.template.rootcow_img)
-        except OSError:
-            # The only case when rootcow_img doesn't exists is in the middle of
-            # commit_changes, so VM is outdated right now
-            return True
-
-        current_dmdev = "/dev/mapper/snapshot-{0:x}:{1}-{2:x}:{3}".format(
-                rootimg_inode[2], rootimg_inode[1],
-                rootcow_inode[2], rootcow_inode[1])
-
-        # FIXME
-        # 51712 (0xCA00) is xvda
-        #  backend node name not available through xenapi :(
-        used_dmdev = self.app.vmm.xs.read('',
-            '/local/domain/0/backend/vbd/{}/51712/node'.format(self.xid))
-
-        return used_dmdev != current_dmdev
-
     #
     # helper methods
     #
diff --git a/qubes/vm/templatevm.py b/qubes/vm/templatevm.py
index 89128ba3..7c4d62bb 100644
--- a/qubes/vm/templatevm.py
+++ b/qubes/vm/templatevm.py
@@ -99,13 +99,3 @@ class TemplateVM(QubesVM):
             }
         }
         super(TemplateVM, self).__init__(*args, **kwargs)
-
-    def commit_changes(self):
-        '''Commit changes to template'''
-        self.log.debug('commit_changes()')
-
-        if not self.app.vmm.offline_mode:
-            assert not self.is_running(), \
-                'Attempt to commit changes on running Template VM!'
-
-        self.storage.commit()