storage: use direct object references, not only identifiers

Reference objects, not their IDs - this way when object is modified, it
is visible everywhere where it is used. Main changes:
- volume.pool - Pool object
- volume.source - Volume object

Since volume have Pool object reference now, move volume related
functions into Volume class (from Pool class). This avoids horrible
`storage.get_pool(volume).something(volume)` construct.

One issue here is since volume.source reference a Volume object from a
different VM - VM's template, now VM load order is important. Since we
don't have control over it, initialize vm.storage when needed - possibly
while initializing storage of different VM. Since we don't have cycles
in AppVM-TemplateVM dependencies, it is safe.

Also, since this commit, volume.source (if defined) always points at
volume of the same name from VM's template. Using volumes with something
else as a source is no longer supported.

QubesOS/qubes-issues#2256

qubes/app.py

@@ -59,6 +59,7 @@ else:
 import qubes
 import qubes.ext
 import qubes.utils
+import qubes.storage
 import qubes.vm
 import qubes.vm.adminvm
 import qubes.vm.qubesvm
@@ -1019,12 +1020,14 @@ class Qubes(qubes.PropertyHolder):
             return
 
 
-    def get_pool(self, name):
+    def get_pool(self, pool):
         '''  Returns a :py:class:`qubes.storage.Pool` instance '''
+        if isinstance(pool, qubes.storage.Pool):
+            return pool
         try:
-            return self.pools[name]
+            return self.pools[pool]
         except KeyError:
-            raise qubes.exc.QubesException('Unknown storage pool ' + name)
+            raise qubes.exc.QubesException('Unknown storage pool ' + pool)
 
     @staticmethod
     def _get_pool(**kwargs):

qubes/storage/__init__.py

@@ -86,7 +86,7 @@ class Volume(object):
         ''' Initialize a volume.
 
             :param str name: The domain name
-            :param str pool: The pool name
+            :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
@@ -96,15 +96,18 @@ class Volume(object):
             :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 str source: Vid of other volume in same pool
+            :param Volume source: other volume in same pool, or None
             :param str/int size: Size of the volume
 
         '''
 
         super(Volume, self).__init__(**kwargs)
+        assert isinstance(pool, Pool)
+        assert source is None or (isinstance(source, Volume)
+                                  and source.pool == pool)
 
         self.name = str(name)
-        self.pool = str(pool)
+        self.pool = pool
         self.internal = internal
         self.removable = removable
         self.revisions_to_keep = int(revisions_to_keep)
@@ -116,7 +119,9 @@ class Volume(object):
         self.vid = vid
 
     def __eq__(self, other):
-        return other.pool == self.pool and other.vid == self.vid
+        if isinstance(other, Volume):
+            return other.pool == self.pool and other.vid == self.vid
+        return NotImplemented
 
     def __hash__(self):
         return hash('%s:%s' % (self.pool, self.vid))
@@ -125,7 +130,7 @@ class Volume(object):
         return not self.__eq__(other)
 
     def __repr__(self):
-        return '{!r}'.format(self.pool + ':' + self.vid)
+        return '{!r}'.format(str(self.pool) + ':' + self.vid)
 
     def __str__(self):
         return str(self.vid)
@@ -134,6 +139,93 @@ class Volume(object):
         config = _sanitize_config(self.config)
         return lxml.etree.Element('volume', **config)
 
+    def create(self):
+        ''' Create the given volume on disk.
+
+            This can be implemented as a coroutine.
+        '''
+        raise self._not_implemented("create")
+
+    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")
+
+    def import_data(self):
+        ''' Returns an object that can be `open()`. '''
+        raise self._not_implemented("import")
+
+    def import_data_end(self, success):
+        ''' End data import operation. This may be used by pool
+        implementation to commit changes, cleanup temporary files etc.
+
+        :param success: True if data import was successful, otherwise False
+        '''
+        # by default do nothing
+        pass
+
+    def import_volume(self, src_volume):
+        ''' Imports data from a different volume (possibly in a different
+        pool '''
+        # pylint: disable=unused-argument
+        raise self._not_implemented("import_volume")
+
+    def is_dirty(self):
+        ''' Return `True` if volume was not properly shutdown and commited '''
+        raise self._not_implemented("is_dirty")
+
+    def is_outdated(self):
+        ''' Returns `True` if the currently used `volume.source` of a snapshot
+            volume is outdated.
+        '''
+        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
+            given size is less than current_size
+
+            This can be implemented as a coroutine.
+        '''
+        # pylint: disable=unused-argument
+        raise self._not_implemented("resize")
+
+    def revert(self, revision=None):
+        ''' Revert volume to previous revision  '''
+        # pylint: disable=unused-argument
+        raise self._not_implemented("revert")
+
+    def start(self):
+        ''' Do what ever is needed on start
+
+        This can be implemented as a coroutine.'''
+        raise self._not_implemented("start")
+
+    def stop(self):
+        ''' Do what ever is needed on stop
+
+        This can be implemented as a coroutine.'''
+
+    def verify(self):
+        ''' Verifies the volume.
+
+        This can be implemented as a coroutine.'''
+        raise self._not_implemented("verify")
+
     def block_device(self):
         ''' Return :py:class:`BlockDevice` for serialization in
             the libvirt XML template as <disk>.
@@ -160,7 +252,7 @@ class Volume(object):
     @property
     def config(self):
         ''' return config data for serialization to qubes.xml '''
-        result = {'name': self.name, 'pool': self.pool, 'vid': self.vid, }
+        result = {'name': self.name, 'pool': str(self.pool), 'vid': self.vid, }
 
         if self.internal:
             result['internal'] = self.internal
@@ -184,10 +276,15 @@ class Volume(object):
             result['snap_on_start'] = self.snap_on_start
 
         if self.source:
-            result['source'] = self.source
+            result['source'] = str(self.source)
 
         return result
 
+    def _not_implemented(self, method_name):
+        ''' Helper for emitting helpful `NotImplementedError` exceptions '''
+        msg = "Volume {!s} has {!s}() not implemented"
+        msg = msg.format(str(self.__class__.__name__), method_name)
+        return NotImplementedError(msg)
 
 class Storage(object):
     ''' Class for handling VM virtual disks.
@@ -204,12 +301,20 @@ class Storage(object):
         self.log = self.vm.log
         #: Additional drive (currently used only by HVM)
         self.drive = None
-        self.pools = {}
 
         if hasattr(vm, 'volume_config'):
             for name, conf in self.vm.volume_config.items():
-                if 'volume_type' in conf:
+                if 'source' in conf:
-                    conf = self._migrate_config(conf)
+                    template = getattr(vm, 'template', None)
+                    if template:
+                        # we have no control over VM load order,
+                        # so initialize storage recursively if needed
+                        if template.storage is None:
+                            template.storage = Storage(template)
+                        # FIXME: this effectively ignore 'source' value;
+                        # maybe we don't need it at all if it's always from
+                        # VM's template?
+                        conf['source'] = template.volumes[name]
 
                 self.init_volume(name, conf)
 
@@ -223,50 +328,8 @@ class Storage(object):
         pool = self.vm.app.get_pool(volume_config['pool'])
         volume = pool.init_volume(self.vm, volume_config)
         self.vm.volumes[name] = volume
-        self.pools[name] = pool
         return volume
 
-    def _migrate_config(self, conf):
-        ''' Migrates from the old config style to new
-        '''  # FIXME: Remove this compatibility hack
-        assert 'volume_type' in conf
-        _type = conf['volume_type']
-        old_volume_types = [
-            'read-write', 'read-only', 'origin', 'snapshot', 'volatile'
-        ]
-        msg = "Volume {!s} has unknown type {!s}".format(conf['name'], _type)
-        assert conf['volume_type'] in old_volume_types, msg
-        if _type == 'origin':
-            conf['rw'] = True
-            conf['source'] = None
-            conf['save_on_stop'] = True
-            conf['revisions_to_keep'] = 1
-        elif _type == 'snapshot':
-            conf['rw'] = False
-            if conf['pool'] == 'default':
-                template_vid = os.path.join('vm-templates',
-                    self.vm.template.name, conf['name'])
-            elif conf['pool'] == 'qubes_dom0':
-                template_vid = os.path.join(
-                    'qubes_dom0', self.vm.template.name + '-' + conf['name'])
-            conf['source'] = template_vid
-            conf['snap_on_start'] = True
-        elif _type == 'read-write':
-            conf['rw'] = True
-            conf['save_on_stop'] = True
-            conf['revisions_to_keep'] = 0
-        elif _type == 'read-only':
-            conf['rw'] = False
-            conf['snap_on_start'] = True
-            conf['save_on_stop'] = False
-            conf['revisions_to_keep'] = 0
-        elif _type == 'volatile':
-            conf['snap_on_start'] = False
-            conf['save_on_stop'] = False
-            conf['revisions_to_keep'] = 0
-        del conf['volume_type']
-        return conf
-
     def attach(self, volume, rw=False):
         ''' Attach a volume to the domain '''
         assert self.vm.is_running()
@@ -342,7 +405,7 @@ class Storage(object):
         ''' Resizes volume a read-writable volume '''
         if isinstance(volume, str):
             volume = self.vm.volumes[volume]
-        ret = self.get_pool(volume).resize(volume, size)
+        ret = volume.resize(size)
         if asyncio.iscoroutine(ret):
             yield from ret
         if self.vm.is_running():
@@ -359,7 +422,7 @@ class Storage(object):
         for volume in self.vm.volumes.values():
             # launch the operation, if it's asynchronous, then append to wait
             #  for them at the end
-            ret = self.get_pool(volume).create(volume)
+            ret = volume.create()
             if asyncio.iscoroutine(ret):
                 coros.append(ret)
         if coros:
@@ -378,10 +441,10 @@ class Storage(object):
         self.vm.volumes = {}
         with VmCreationManager(self.vm):
             for name, config in self.vm.volume_config.items():
-                dst_pool = self.get_pool(config['pool'])
+                dst_pool = self.vm.app.get_pool(config['pool'])
                 dst = dst_pool.init_volume(self.vm, config)
                 src_volume = src_vm.volumes[name]
-                src_pool = self.vm.app.get_pool(src_volume.pool)
+                src_pool = src_volume.pool
                 if dst_pool == src_pool:
                     msg = "Cloning volume {!s} from vm {!s}"
                     self.vm.log.info(msg.format(src_volume.name, src_vm.name))
@@ -404,7 +467,7 @@ class Storage(object):
                     volume = clone_op_ret
 
                 assert volume, "%s.clone() returned '%s'" % (
-                    self.get_pool(self.vm.volume_config[name]['pool']).
+                    self.vm.app.get_pool(self.vm.volume_config[name]['pool']).
                         __class__.__name__, volume)
 
                 self.vm.volumes[name] = volume
@@ -418,8 +481,7 @@ class Storage(object):
 
         volumes = self.vm.volumes
         for volume in volumes.values():
-            pool = self.get_pool(volume)
+            if volume.is_outdated():
-            if pool.is_outdated(volume):
                 result += [volume]
 
         return result
@@ -428,7 +490,7 @@ class Storage(object):
         ''' Notify the pools that the domain was renamed '''
         volumes = self.vm.volumes
         for name, volume in volumes.items():
-            pool = self.get_pool(volume)
+            pool = volume.pool
             volumes[name] = pool.rename(volume, old_name, new_name)
 
     @asyncio.coroutine
@@ -443,7 +505,7 @@ class Storage(object):
                 'VM directory does not exist: {}'.format(self.vm.dir_path))
         futures = []
         for volume in self.vm.volumes.values():
-            ret = self.get_pool(volume).verify(volume)
+            ret = volume.verify()
             if asyncio.iscoroutine(ret):
                 futures.append(ret)
         if futures:
@@ -461,7 +523,7 @@ class Storage(object):
         for name, volume in self.vm.volumes.items():
             self.log.info('Removing volume %s: %s' % (name, volume.vid))
             try:
-                ret = self.get_pool(volume).remove(volume)
+                ret = volume.pool.remove(volume)
                 if asyncio.iscoroutine(ret):
                     futures.append(ret)
             except (IOError, OSError) as e:
@@ -478,8 +540,7 @@ class Storage(object):
         ''' Execute the start method on each pool '''
         futures = []
         for volume in self.vm.volumes.values():
-            pool = self.get_pool(volume)
+            ret = volume.start()
-            ret = pool.start(volume)
             if asyncio.iscoroutine(ret):
                 futures.append(ret)
 
@@ -491,29 +552,20 @@ class Storage(object):
         ''' Execute the start method on each pool '''
         futures = []
         for volume in self.vm.volumes.values():
-            ret = self.get_pool(volume).stop(volume)
+            ret = volume.stop()
             if asyncio.iscoroutine(ret):
                 futures.append(ret)
 
         if futures:
             yield from asyncio.wait(futures)
 
-    def get_pool(self, volume):
-        ''' Helper function '''
-        assert isinstance(volume, (Volume, str)), \
-            "You need to pass a Volume or pool name as str"
-        if isinstance(volume, Volume):
-            return self.pools[volume.name]
-
-        return self.vm.app.pools[volume]
-
     @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 = self.get_pool(volume).commit(volume)
+                ret = volume.commit()
                 if asyncio.iscoroutine(ret):
                     futures.append(ret)
 
@@ -540,18 +592,18 @@ class Storage(object):
         assert isinstance(volume, (Volume, str)), \
             "You need to pass a Volume or pool name as str"
         if isinstance(volume, Volume):
-            return self.pools[volume.name].export(volume)
+            return volume.export()
 
-        return self.pools[volume].export(self.vm.volumes[volume])
+        return self.vm.volumes[volume].export()
 
     def import_data(self, volume):
         ''' Helper function to import volume data (pool.import_data(volume))'''
         assert isinstance(volume, (Volume, str)), \
             "You need to pass a Volume or pool name as str"
         if isinstance(volume, Volume):
-            return self.pools[volume.name].import_data(volume)
+            return volume.import_data()
 
-        return self.pools[volume].import_data(self.vm.volumes[volume])
+        return self.vm.volumes[volume].import_data()
 
     def import_data_end(self, volume, success):
         ''' Helper function to finish/cleanup data import
@@ -559,11 +611,10 @@ class Storage(object):
         assert isinstance(volume, (Volume, str)), \
             "You need to pass a Volume or pool name as str"
         if isinstance(volume, Volume):
-            return self.pools[volume.name].import_data_end(volume,
+            return volume.import_data_end(volume,
                 success=success)
 
-        return self.pools[volume].import_data_end(self.vm.volumes[volume],
+        return self.vm.volumes[volume].import_data_end(success=success)
-            success=success)
 
 
 class Pool(object):
@@ -583,7 +634,11 @@ class Pool(object):
         kwargs['name'] = self.name
 
     def __eq__(self, other):
-        return self.name == other.name
+        if isinstance(other, Pool):
+            return self.name == other.name
+        elif isinstance(other, str):
+            return self.name == other
+        return NotImplemented
 
     def __neq__(self, other):
         return not self.__eq__(other)
@@ -598,79 +653,22 @@ class Pool(object):
         config = _sanitize_config(self.config)
         return lxml.etree.Element('pool', **config)
 
-    def create(self, volume):
-        ''' Create the given volume on disk or copy from provided
-            `source_volume`.
-
-            This can be implemented as a coroutine.
-        '''
-        raise self._not_implemented("create")
-
-    def commit(self, volume):  # pylint: disable=no-self-use
-        ''' Write the snapshot to disk
-
-        This can be implemented as a coroutine.'''
-        msg = "Got volume_type {!s} when expected 'snap'"
-        msg = msg.format(volume.volume_type)
-        assert volume.volume_type == 'snap', msg
-
     @property
     def config(self):
         ''' Returns the pool config to be written to qubes.xml '''
         raise self._not_implemented("config")
 
-    def clone(self, source, target):
-        ''' Clone volume.
-
-        This can be implemented as a coroutine. '''
-        raise self._not_implemented("clone")
-
     def destroy(self):
         ''' Called when removing the pool. Use this for implementation specific
             clean up.
         '''
         raise self._not_implemented("destroy")
 
-    def export(self, volume):
-        ''' Returns an object that can be `open()`. '''
-        raise self._not_implemented("export")
-
-    def import_data(self, volume):
-        ''' Returns an object that can be `open()`. '''
-        raise self._not_implemented("import")
-
-    def import_data_end(self, volume, success):
-        ''' End data import operation. This may be used by pool
-        implementation to commit changes, cleanup temporary files etc.
-
-        :param success: True if data import was successful, otherwise False
-        '''
-        # by default do nothing
-        pass
-
-    def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
-        ''' Imports data to a volume in this pool '''
-        raise self._not_implemented("import_volume")
-
     def init_volume(self, vm, volume_config):
         ''' Initialize a :py:class:`qubes.storage.Volume` from `volume_config`.
         '''
         raise self._not_implemented("init_volume")
 
-    def is_dirty(self, volume):
-        ''' Return `True` if volume was not properly shutdown and commited '''
-        raise self._not_implemented("is_dirty")
-
-    def is_outdated(self, volume):
-        ''' Returns `True` if the currently used `volume.source` of a snapshot
-            volume is outdated.
-        '''
-        raise self._not_implemented("is_outdated")
-
-    def recover(self, volume):
-        ''' Try to recover a :py:class:`Volume` or :py:class:`SnapVolume` '''
-        raise self._not_implemented("recover")
-
     def remove(self, volume):
         ''' Remove volume.
 
@@ -681,47 +679,12 @@ class Pool(object):
         ''' Called when the domain changes its name '''
         raise self._not_implemented("rename")
 
-    def reset(self, volume):
-        ''' Drop and recreate volume without copying it's content from source.
-        '''
-        raise self._not_implemented("reset")
-
-    def resize(self, volume, size):
-        ''' Expands volume, throws
-            :py:class:`qubes.storage.StoragePoolException` if
-            given size is less than current_size
-
-            This can be implemented as a coroutine.
-        '''
-        raise self._not_implemented("resize")
-
-    def revert(self, volume, revision=None):
-        ''' Revert volume to previous revision  '''
-        raise self._not_implemented("revert")
-
     def setup(self):
         ''' Called when adding a pool to the system. Use this for implementation
             specific set up.
         '''
         raise self._not_implemented("setup")
 
-    def start(self, volume):  # pylint: disable=no-self-use
-        ''' Do what ever is needed on start
-
-        This can be implemented as a coroutine.'''
-        raise self._not_implemented("start")
-
-    def stop(self, volume):  # pylint: disable=no-self-use
-        ''' Do what ever is needed on stop
-
-        This can be implemented as a coroutine.'''
-
-    def verify(self, volume):
-        ''' Verifies the volume.
-
-        This can be implemented as a coroutine.'''
-        raise self._not_implemented("verify")
-
     @property
     def volumes(self):
         ''' Return a list of volumes managed by this pool '''
@@ -780,7 +743,7 @@ class VmCreationManager(object):
         if type is not None and value is not None and tb is not None:
             for volume in self.vm.volumes.values():
                 try:
-                    pool = self.vm.storage.get_pool(volume)
+                    pool = volume.pool
                     pool.remove(volume)
                 except Exception:  # pylint: disable=broad-except
                     pass

qubes/storage/file.py

@@ -56,30 +56,6 @@ class FilePool(qubes.storage.Pool):
             'revisions_to_keep': self.revisions_to_keep
         }
 
-    def clone(self, source, target):
-        new_dir = os.path.dirname(target.path)
-        if target._is_origin or target._is_volume:
-            if not os.path.exists:
-                os.makedirs(new_dir)
-            copy_file(source.path, target.path)
-        return target
-
-    def create(self, volume):
-        assert isinstance(volume.size, int) and volume.size > 0, \
-            'Volatile volume size must be > 0'
-        if volume._is_origin:
-            create_sparse_file(volume.path, volume.size)
-            create_sparse_file(volume.path_cow, volume.size)
-        elif not volume._is_snapshot:
-            if volume.source is not None:
-                source_path = os.path.join(self.dir_path,
-                                           volume.source + '.img')
-                copy_file(source_path, volume.path)
-            elif volume._is_volatile:
-                pass
-            else:
-                create_sparse_file(volume.path, volume.size)
-
     def init_volume(self, vm, volume_config):
         volume_config['dir_path'] = self.dir_path
         if os.path.join(self.dir_path, self._vid_prefix(vm)) == vm.dir_path:
@@ -98,45 +74,11 @@ class FilePool(qubes.storage.Pool):
             if 'revisions_to_keep' not in volume_config:
                 volume_config['revisions_to_keep'] = self.revisions_to_keep
 
+        volume_config['pool'] = self
         volume = FileVolume(**volume_config)
         self._volumes += [volume]
         return volume
 
-    def is_dirty(self, volume):
-        return False  # TODO: How to implement this?
-
-    def resize(self, volume, size):
-        ''' Expands volume, throws
-            :py:class:`qubst.storage.qubes.storage.StoragePoolException` if
-            given size is less than current_size
-        '''  # pylint: disable=no-self-use
-        if not volume.rw:
-            msg = 'Can not resize reađonly volume {!s}'.format(volume)
-            raise qubes.storage.StoragePoolException(msg)
-
-        if size <= volume.size:
-            raise qubes.storage.StoragePoolException(
-                'For your own safety, shrinking of %s is'
-                ' disabled. If you really know what you'
-                ' are doing, use `truncate` on %s manually.' %
-                (volume.name, volume.vid))
-
-        with open(volume.path, 'a+b') as fd:
-            fd.truncate(size)
-
-        p = subprocess.Popen(['sudo', 'losetup', '--associated', volume.path],
-                             stdout=subprocess.PIPE)
-        result = p.communicate()
-
-        m = re.match(r'^(/dev/loop\d+):\s', result[0].decode())
-        if m is not None:
-            loop_dev = m.group(1)
-
-            # resize loop device
-            subprocess.check_call(['sudo', 'losetup', '--set-capacity',
-                                   loop_dev])
-        volume.size = size
-
     def remove(self, volume):
         if not volume.internal:
             return  # do not remove random attached file volumes
@@ -167,68 +109,9 @@ class FilePool(qubes.storage.Pool):
 
         return volume
 
-    def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
-        msg = "Can not import snapshot volume {!s} in to pool {!s} "
-        msg = msg.format(src_volume, self)
-        assert not src_volume.snap_on_start, msg
-        if dst_volume.save_on_stop:
-            copy_file(src_pool.export(src_volume), dst_volume.path)
-        return dst_volume
-
-    def commit(self, volume):
-        msg = 'Tried to commit a non commitable volume {!r}'.format(volume)
-        assert (volume._is_origin or volume._is_volume) and volume.rw, msg
-
-        if volume._is_volume:
-            return volume
-
-        if os.path.exists(volume.path_cow):
-            old_path = volume.path_cow + '.old'
-            os.rename(volume.path_cow, old_path)
-
-        old_umask = os.umask(0o002)
-        with open(volume.path_cow, 'w') as f_cow:
-            f_cow.truncate(volume.size)
-        os.umask(old_umask)
-        return volume
-
     def destroy(self):
         pass
 
-    def export(self, volume):
-        return volume.path
-
-    def import_data(self, volume):
-        return volume.path
-
-    def reset(self, volume):
-        ''' Remove and recreate a volatile volume '''
-        assert volume._is_volatile, "Not a volatile volume"
-        assert isinstance(volume.size, int) and volume.size > 0, \
-            'Volatile volume size must be > 0'
-
-        _remove_if_exists(volume.path)
-
-        with open(volume.path, "w") as f_volatile:
-            f_volatile.truncate(volume.size)
-        return volume
-
-    def revert(self, volume, revision=None):
-        if revision is not None:
-            try:
-                return volume.revisions[revision]
-            except KeyError:
-                msg = "Volume {!r} does not have revision {!s}"
-                msg = msg.format(volume, revision)
-                raise qubes.storage.StoragePoolException(msg)
-        else:
-            try:
-                old_path = volume.revisions.values().pop()
-                os.rename(old_path, volume.path_cow)
-            except IndexError:
-                msg = "Volume {!r} does not have old revisions".format(volume)
-                raise qubes.storage.StoragePoolException(msg)
-
     def setup(self):
         create_dir_if_not_exists(self.dir_path)
         appvms_path = os.path.join(self.dir_path, 'appvms')
@@ -236,38 +119,6 @@ class FilePool(qubes.storage.Pool):
         vm_templates_path = os.path.join(self.dir_path, 'vm-templates')
         create_dir_if_not_exists(vm_templates_path)
 
-    def start(self, volume):
-        if volume._is_volatile:
-            self.reset(volume)
-        else:
-            _check_path(volume.path)
-            if volume.snap_on_start:
-                if not volume.save_on_stop:
-                    # make sure previous snapshot is removed - even if VM
-                    # shutdown routing wasn't called (power interrupt or so)
-                    _remove_if_exists(volume.path_cow)
-                try:
-                    _check_path(volume.path_cow)
-                except qubes.storage.StoragePoolException:
-                    create_sparse_file(volume.path_cow, volume.size)
-                    _check_path(volume.path_cow)
-                if hasattr(volume, 'path_source_cow'):
-                    try:
-                        _check_path(volume.path_source_cow)
-                    except qubes.storage.StoragePoolException:
-                        create_sparse_file(volume.path_source_cow, volume.size)
-                        _check_path(volume.path_source_cow)
-        return volume
-
-    def stop(self, volume):
-        if volume.save_on_stop:
-            self.commit(volume)
-        elif volume.snap_on_start:
-            _remove_if_exists(volume.path_cow)
-        else:
-            _remove_if_exists(volume.path)
-        return volume
-
     @staticmethod
     def _vid_prefix(vm):
         ''' Helper to create a prefix for the vid for volume
@@ -301,9 +152,6 @@ class FilePool(qubes.storage.Pool):
 
         return os.path.join(self.dir_path, self._vid_prefix(vm))
 
-    def verify(self, volume):
-        return volume.verify()
-
     @property
     def volumes(self):
         return self._volumes
@@ -329,7 +177,7 @@ class FileVolume(qubes.storage.Volume):
             raise qubes.storage.StoragePoolException(msg)
 
         if self._is_snapshot:
-            img_name = self.source + '-cow.img'
+            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
@@ -338,10 +186,153 @@ class FileVolume(qubes.storage.Volume):
         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:
+            create_sparse_file(self.path, self.size)
+            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 is_dirty(self):
+        return False  # TODO: How to implement this?
+
+    def resize(self, size):
+        ''' Expands volume, throws
+            :py:class:`qubst.storage.qubes.storage.StoragePoolException` if
+            given size is less than current_size
+        '''  # pylint: disable=no-self-use
+        if not self.rw:
+            msg = 'Can not resize reađonly volume {!s}'.format(self)
+            raise qubes.storage.StoragePoolException(msg)
+
+        if size <= self.size:
+            raise qubes.storage.StoragePoolException(
+                'For your own safety, shrinking of %s is'
+                ' disabled. If you really know what you'
+                ' are doing, use `truncate` on %s manually.' %
+                (self.name, self.vid))
+
+        with open(self.path, 'a+b') as fd:
+            fd.truncate(size)
+
+        p = subprocess.Popen(['sudo', 'losetup', '--associated', self.path],
+                             stdout=subprocess.PIPE)
+        result = p.communicate()
+
+        m = re.match(r'^(/dev/loop\d+):\s', result[0].decode())
+        if m is not None:
+            loop_dev = m.group(1)
+
+            # resize loop device
+            subprocess.check_call(['sudo', '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
+
+        if os.path.exists(self.path_cow):
+            old_path = self.path_cow + '.old'
+            os.rename(self.path_cow, old_path)
+
+        old_umask = os.umask(0o002)
+        with open(self.path_cow, 'w') as f_cow:
+            f_cow.truncate(self.size)
+        os.umask(old_umask)
+        return self
+
+    def export(self):
+        return self.path
+
+    def import_volume(self, src_volume):
+        msg = "Can not import snapshot volume {!s} in to pool {!s} "
+        msg = msg.format(src_volume, self)
+        assert not src_volume.snap_on_start, msg
+        if self.save_on_stop:
+            copy_file(src_volume.export(), self.path)
+        return self
+
+
+    def import_data(self):
+        return self.path
+
+    def reset(self):
+        ''' Remove and recreate a volatile volume '''
+        assert self._is_volatile, "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)
+        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:
+            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)
+        return self
+
+    def stop(self):
+        if self.save_on_stop:
+            self.commit()
+        elif self.snap_on_start:
+            _remove_if_exists(self.path_cow)
+        else:
+            _remove_if_exists(self.path)
+        return self
+
     @property
     def path(self):
         if self._is_snapshot:
-            return os.path.join(self.dir_path, self.source + '.img')
+            return os.path.join(self.dir_path, self.source.vid + '.img')
         return os.path.join(self.dir_path, self.vid + '.img')
 
     @property

qubes/storage/kernels.py

@@ -33,7 +33,6 @@ class LinuxModules(Volume):
 
     def __init__(self, target_dir, kernel_version, **kwargs):
         kwargs['vid'] = ''
-        kwargs['source'] = self
         super(LinuxModules, self).__init__(**kwargs)
         self._kernel_version = kernel_version
         self.target_dir = target_dir
@@ -81,6 +80,44 @@ class LinuxModules(Volume):
     def revisions(self):
         return {}
 
+    def is_dirty(self):
+        return False
+
+    def clone(self, source):
+        if isinstance(source, LinuxModules):
+            # do nothing
+            return self
+        raise StoragePoolException('clone of LinuxModules volume from '
+                                  'different volume type is not supported')
+
+    def create(self):
+        return self
+
+    def commit(self):
+        return self
+
+    def export(self):
+        return self.path
+
+    def is_outdated(self):
+        return False
+
+    def start(self):
+        path = self.path
+        if path and not os.path.exists(path):
+            raise StoragePoolException('Missing kernel modules: %s' % path)
+
+        return self
+
+    def stop(self):
+        pass
+
+    def verify(self):
+        if self.vid:
+            _check_path(self.path)
+            _check_path(self.vmlinuz)
+            _check_path(self.initramfs)
+
     def block_device(self):
         if self.vid:
             return super().block_device()
@@ -98,22 +135,11 @@ class LinuxKernel(Pool):
     def init_volume(self, vm, volume_config):
         assert not volume_config['rw']
 
+        volume_config['pool'] = self
         volume = LinuxModules(self.dir_path, lambda: vm.kernel, **volume_config)
 
         return volume
 
-    def is_dirty(self, volume):
-        return False
-
-    def clone(self, source, target):
-        return target
-
-    def create(self, volume):
-        return volume
-
-    def commit(self, volume):
-        return volume
-
     @property
     def config(self):
         return {
@@ -125,15 +151,9 @@ class LinuxKernel(Pool):
     def destroy(self):
         pass
 
-    def export(self, volume):
-        return volume.path
-
     def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
         pass
 
-    def is_outdated(self, volume):
-        return False
-
     def remove(self, volume):
         pass
 
@@ -143,28 +163,12 @@ class LinuxKernel(Pool):
     def setup(self):
         pass
 
-    def start(self, volume):
-        path = volume.path
-        if path and not os.path.exists(path):
-            raise StoragePoolException('Missing kernel modules: %s' % path)
-
-        return volume
-
-    def stop(self, volume):
-        pass
-
-    def verify(self, volume):
-        if volume.vid:
-            _check_path(volume.path)
-            _check_path(volume.vmlinuz)
-            _check_path(volume.initramfs)
-
     @property
     def volumes(self):
         ''' Return all known kernel volumes '''
         return [LinuxModules(self.dir_path,
                              kernel_version,
-                             pool=self.name,
+                             pool=self,
                              name=kernel_version,
                              internal=True,
                              rw=False

qubes/storage/lvm.py

@@ -25,6 +25,8 @@ import os
 import subprocess
 
 import qubes
+import qubes.storage
+import qubes.utils
 
 
 def check_lvm_version():
@@ -54,34 +56,6 @@ class ThinPool(qubes.storage.Pool):
         self._pool_id = "{!s}/{!s}".format(volume_group, thin_pool)
         self.log = logging.getLogger('qube.storage.lvm.%s' % self._pool_id)
 
-    def clone(self, source, target):
-        cmd = ['clone', str(source), str(target)]
-        qubes_lvm(cmd, self.log)
-        return target
-
-    def _commit(self, volume):
-        msg = "Trying to commit {!s}, but it has save_on_stop == False"
-        msg = msg.format(volume)
-        assert volume.save_on_stop, msg
-
-        msg = "Trying to commit {!s}, but it has rw == False"
-        msg = msg.format(volume)
-        assert volume.rw, msg
-        assert hasattr(volume, '_vid_snap')
-
-        try:
-            cmd = ['remove', volume.vid + "-back"]
-            qubes_lvm(cmd, self.log)
-        except qubes.storage.StoragePoolException:
-            pass
-        cmd = ['clone', volume.vid, volume.vid + "-back"]
-        qubes_lvm(cmd, self.log)
-
-        cmd = ['remove', volume.vid]
-        qubes_lvm(cmd, self.log)
-        cmd = ['clone', volume._vid_snap, volume.vid]
-        qubes_lvm(cmd, self.log)
-
     @property
     def config(self):
         return {
@@ -91,36 +65,9 @@ class ThinPool(qubes.storage.Pool):
             'driver': ThinPool.driver
         }
 
-    def create(self, volume):
-        assert volume.vid
-        assert volume.size
-        if volume.save_on_stop:
-            if volume.source:
-                cmd = ['clone', str(volume.source), volume.vid]
-            else:
-                cmd = [
-                    'create',
-                    self._pool_id,
-                    volume.vid.split('/', 1)[1],
-                    str(volume.size)
-                ]
-            qubes_lvm(cmd, self.log)
-            reset_cache()
-        return volume
-
     def destroy(self):
         pass  # TODO Should we remove an existing pool?
 
-    def export(self, volume):
-        ''' Returns an object that can be `open()`. '''
-        devpath = '/dev/' + volume.vid
-        return devpath
-
-    def import_data(self, volume):
-        ''' Returns an object that can be `open()`. '''
-        devpath = '/dev/' + volume.vid
-        return devpath
-
     def init_volume(self, vm, volume_config):
         ''' Initialize a :py:class:`qubes.storage.Volume` from `volume_config`.
         '''
@@ -138,37 +85,12 @@ class ThinPool(qubes.storage.Pool):
                 self.volume_group, vm_name, volume_config['name'])
 
         volume_config['volume_group'] = self.volume_group
-
+        volume_config['pool'] = self
         return ThinVolume(**volume_config)
 
-    def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
-        if not src_volume.save_on_stop:
-            return dst_volume
-
-        src_path = src_pool.export(src_volume)
-
-        # HACK: neat trick to speed up testing if you have same physical thin
-        # pool assigned to two qubes-pools i.e: qubes_dom0 and test-lvm
-        # pylint: disable=line-too-long
-        if isinstance(src_pool, ThinPool) and src_pool.thin_pool == dst_pool.thin_pool:  # NOQA
-            return self.clone(src_volume, dst_volume)
-        else:
-            dst_volume = self.create(dst_volume)
-
-        cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + dst_volume.vid,
-            'conv=sparse']
-        subprocess.check_call(cmd)
-        reset_cache()
-        return dst_volume
-
-    def is_dirty(self, volume):
-        if volume.save_on_stop:
-            return os.path.exists(volume.path + '-snap')
-        return False
-
     def remove(self, volume):
         assert volume.vid
-        if self.is_dirty(volume):
+        if volume.is_dirty():
             cmd = ['remove', volume._vid_snap]
             qubes_lvm(cmd, self.log)
 
@@ -195,85 +117,9 @@ class ThinPool(qubes.storage.Pool):
         reset_cache()
         return volume
 
-    def revert(self, volume, revision=None):
-        old_path = volume.path + '-back'
-        if not os.path.exists(old_path):
-            msg = "Volume {!s} has no {!s}".format(volume, old_path)
-            raise qubes.storage.StoragePoolException(msg)
-
-        cmd = ['remove', volume.vid]
-        qubes_lvm(cmd, self.log)
-        cmd = ['clone', volume.vid + '-back', volume.vid]
-        qubes_lvm(cmd, self.log)
-        reset_cache()
-        return volume
-
-    def resize(self, volume, size):
-        ''' Expands volume, throws
-            :py:class:`qubst.storage.qubes.storage.StoragePoolException` if
-            given size is less than current_size
-        '''
-        if not volume.rw:
-            msg = 'Can not resize reađonly volume {!s}'.format(volume)
-            raise qubes.storage.StoragePoolException(msg)
-
-        if size <= volume.size:
-            raise qubes.storage.StoragePoolException(
-                'For your own safety, shrinking of %s is'
-                ' disabled. If you really know what you'
-                ' are doing, use `lvresize` on %s manually.' %
-                (volume.name, volume.vid))
-
-        cmd = ['extend', volume.vid, str(size)]
-        qubes_lvm(cmd, self.log)
-        reset_cache()
-
     def setup(self):
         pass  # TODO Should we create a non existing pool?
 
-    def start(self, volume):
-        if volume.snap_on_start:
-            if not volume.save_on_stop or not self.is_dirty(volume):
-                self._snapshot(volume)
-        elif not volume.save_on_stop:
-            self._reset_volume(volume)
-
-        reset_cache()
-        return volume
-
-    def stop(self, volume):
-        if volume.save_on_stop and volume.snap_on_start:
-            self._commit(volume)
-        if volume.snap_on_start:
-            cmd = ['remove', volume._vid_snap]
-            qubes_lvm(cmd, self.log)
-        elif not volume.save_on_stop:
-            cmd = ['remove', volume.vid]
-            qubes_lvm(cmd, self.log)
-        reset_cache()
-        return volume
-
-    def _snapshot(self, volume):
-        try:
-            cmd = ['remove', volume._vid_snap]
-            qubes_lvm(cmd, self.log)
-        except:  # pylint: disable=bare-except
-            pass
-
-        if volume.source is None:
-            cmd = ['clone', volume.vid, volume._vid_snap]
-        else:
-            cmd = ['clone', str(volume.source), volume._vid_snap]
-        qubes_lvm(cmd, self.log)
-
-    def verify(self, volume):
-        ''' Verifies the volume. '''
-        try:
-            vol_info = size_cache[volume.vid]
-            return vol_info['attr'][4] == 'a'
-        except KeyError:
-            return False
-
     @property
     def volumes(self):
         ''' Return a list of volumes managed by this pool '''
@@ -296,20 +142,6 @@ class ThinPool(qubes.storage.Pool):
             volumes += [ThinVolume(**config)]
         return volumes
 
-    def _reset_volume(self, volume):
-        ''' Resets a volatile volume '''
-        assert volume._is_volatile, \
-            'Expected a volatile volume, but got {!r}'.format(volume)
-        self.log.debug('Resetting volatile ' + volume.vid)
-        try:
-            cmd = ['remove', volume.vid]
-            qubes_lvm(cmd, self.log)
-        except qubes.storage.StoragePoolException:
-            pass
-        cmd = ['create', self._pool_id, volume.vid.split('/')[1],
-               str(volume.size)]
-        qubes_lvm(cmd, self.log)
-
 
 def init_cache(log=logging.getLogger('qube.storage.lvm')):
     cmd = ['lvs', '--noheadings', '-o',
@@ -352,6 +184,7 @@ class ThinVolume(qubes.storage.Volume):
     def __init__(self, volume_group, size=0, **kwargs):
         self.volume_group = volume_group
         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"
@@ -408,6 +241,178 @@ class ThinVolume(qubes.storage.Volume):
         raise qubes.storage.StoragePoolException(
             "You shouldn't use lvm size setter")
 
+    def _reset(self):
+        ''' Resets a volatile volume '''
+        assert self._is_volatile, \
+            'Expected a volatile volume, but got {!r}'.format(self)
+        self.log.debug('Resetting volatile ' + self.vid)
+        try:
+            cmd = ['remove', self.vid]
+            qubes_lvm(cmd, self.log)
+        except qubes.storage.StoragePoolException:
+            pass
+        # pylint: disable=protected-access
+        cmd = ['create', self.pool._pool_id, self.vid.split('/')[1],
+               str(self.size)]
+        qubes_lvm(cmd, self.log)
+
+    def _commit(self):
+        msg = "Trying to commit {!s}, but it has save_on_stop == False"
+        msg = msg.format(self)
+        assert self.save_on_stop, msg
+
+        msg = "Trying to commit {!s}, but it has rw == False"
+        msg = msg.format(self)
+        assert self.rw, msg
+        assert hasattr(self, '_vid_snap')
+
+        try:
+            cmd = ['remove', self.vid + "-back"]
+            qubes_lvm(cmd, self.log)
+        except qubes.storage.StoragePoolException:
+            pass
+        cmd = ['clone', self.vid, self.vid + "-back"]
+        qubes_lvm(cmd, self.log)
+
+        cmd = ['remove', self.vid]
+        qubes_lvm(cmd, self.log)
+        cmd = ['clone', self._vid_snap, self.vid]
+        qubes_lvm(cmd, self.log)
+
+
+    def create(self):
+        assert self.vid
+        assert self.size
+        if self.save_on_stop:
+            if self.source:
+                cmd = ['clone', str(self.source), self.vid]
+            else:
+                cmd = [
+                    'create',
+                    self.pool._pool_id,  # pylint: disable=protected-access
+                    self.vid.split('/', 1)[1],
+                    str(self.size)
+                ]
+            qubes_lvm(cmd, self.log)
+            reset_cache()
+        return self
+
+    def export(self):
+        ''' Returns an object that can be `open()`. '''
+        devpath = '/dev/' + self.vid
+        return devpath
+
+    def import_volume(self, src_volume):
+        if not src_volume.save_on_stop:
+            return self
+
+        src_path = src_volume.export()
+
+        # HACK: neat trick to speed up testing if you have same physical thin
+        # pool assigned to two qubes-pools i.e: qubes_dom0 and test-lvm
+        # pylint: disable=line-too-long
+        if isinstance(src_volume.pool, ThinPool) and \
+                src_volume.pool.thin_pool == self.pool.thin_pool:  # NOQA
+            cmd = ['clone', str(src_volume), str(self)]
+            qubes_lvm(cmd, self.log)
+        else:
+            self.create()
+
+            cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + self.vid,
+                'conv=sparse']
+            subprocess.check_call(cmd)
+            reset_cache()
+
+        return self
+
+    def import_data(self):
+        ''' Returns an object that can be `open()`. '''
+        devpath = '/dev/' + self.vid
+        return devpath
+
+    def is_dirty(self):
+        if self.save_on_stop:
+            return os.path.exists(self.path + '-snap')
+        return False
+
+    def revert(self, revision=None):
+        old_path = self.path + '-back'
+        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]
+        qubes_lvm(cmd, self.log)
+        reset_cache()
+        return self
+
+    def resize(self, size):
+        ''' Expands volume, throws
+            :py:class:`qubst.storage.qubes.storage.StoragePoolException` if
+            given size is less than current_size
+        '''
+        if not self.rw:
+            msg = 'Can not resize reađonly volume {!s}'.format(self)
+            raise qubes.storage.StoragePoolException(msg)
+
+        if size <= self.size:
+            raise qubes.storage.StoragePoolException(
+                'For your own safety, shrinking of %s is'
+                ' disabled. If you really know what you'
+                ' are doing, use `lvresize` on %s manually.' %
+                (self.name, self.vid))
+
+        cmd = ['extend', self.vid, str(size)]
+        qubes_lvm(cmd, self.log)
+        reset_cache()
+
+    def _snapshot(self):
+        try:
+            cmd = ['remove', self._vid_snap]
+            qubes_lvm(cmd, self.log)
+        except:  # pylint: disable=bare-except
+            pass
+
+        if self.source is None:
+            cmd = ['clone', self.vid, self._vid_snap]
+        else:
+            cmd = ['clone', str(self.source), self._vid_snap]
+        qubes_lvm(cmd, self.log)
+
+
+    def start(self):
+        if self.snap_on_start:
+            if not self.save_on_stop or not self.is_dirty():
+                self._snapshot()
+        elif not self.save_on_stop:
+            self._reset()
+
+        reset_cache()
+        return self
+
+    def stop(self):
+        if self.save_on_stop and self.snap_on_start:
+            self._commit()
+        if self.snap_on_start:
+            cmd = ['remove', self._vid_snap]
+            qubes_lvm(cmd, self.log)
+        elif not self.save_on_stop:
+            cmd = ['remove', self.vid]
+            qubes_lvm(cmd, self.log)
+        reset_cache()
+        return self
+
+    def verify(self):
+        ''' Verifies the volume. '''
+        try:
+            vol_info = size_cache[self.vid]
+            return vol_info['attr'][4] == 'a'
+        except KeyError:
+            return False
+
+
     def block_device(self):
         ''' Return :py:class:`qubes.storage.BlockDevice` for serialization in
             the libvirt XML template as <disk>.
@@ -437,7 +442,7 @@ def pool_exists(pool_id):
         return False
 
 
-def qubes_lvm(cmd, log=logging.getLogger('qube.storage.lvm')):
+def qubes_lvm(cmd, log=logging.getLogger('qubes.storage.lvm')):
     ''' Call :program:`lvm` to execute an LVM operation '''
     action = cmd[0]
     if action == 'remove':

qubes/tests/storage_file.py

@@ -138,22 +138,31 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
         self.assertTrue(volume.rw)
 
     def test_001_snapshot_volume(self):
-        source = 'vm-templates/fedora-23/root'
+        template_vm = self.app.default_template
+        vm = qubes.tests.storage.TestVM(self, template=template_vm)
+
         original_size = qubes.config.defaults['root_img_size']
+        source_config = {
+            'name': 'root',
+            'pool': self.POOL_NAME,
+            'save_on_stop': True,
+            'rw': False,
+            'size': original_size,
+        }
+        source = self.app.get_pool(self.POOL_NAME).init_volume(template_vm,
+            source_config)
         config = {
             'name': 'root',
-            'pool': 'default',
+            'pool': self.POOL_NAME,
             'snap_on_start': True,
             'rw': False,
             'source': source,
             'size': original_size,
         }
 
-        template_vm = self.app.default_template
-        vm = qubes.tests.storage.TestVM(self, template=template_vm)
         volume = self.app.get_pool(self.POOL_NAME).init_volume(vm, config)
         self.assertEqual(volume.name, 'root')
-        self.assertEqual(volume.pool, 'default')
+        self.assertEqual(volume.pool, self.POOL_NAME)
         self.assertEqual(volume.size, original_size)
         self.assertTrue(volume.snap_on_start)
         self.assertTrue(volume.snap_on_start)
@@ -181,12 +190,17 @@ class TC_01_FileVolumes(qubes.tests.QubesTestCase):
     def test_003_read_only_volume(self):
         template = self.app.default_template
         vid = template.volumes['root'].vid
-        config = {'name': 'root', 'pool': 'default', 'rw': False, 'vid': vid}
+        config = {
+            'name': 'root',
+            'pool': self.POOL_NAME,
+            'rw': False,
+            'vid': vid,
+        }
         vm = qubes.tests.storage.TestVM(self, template=template)
 
         volume = self.app.get_pool(self.POOL_NAME).init_volume(vm, config)
         self.assertEqual(volume.name, 'root')
-        self.assertEqual(volume.pool, 'default')
+        self.assertEqual(volume.pool, self.POOL_NAME)
 
         # original_size = qubes.config.defaults['root_img_size']
         # FIXME: self.assertEqual(volume.size, original_size)
@@ -322,10 +336,9 @@ class TC_03_FilePool(qubes.tests.QubesTestCase):
         self.assertEqual(vm.volumes['private'].path, expected_private_path)
 
         expected_volatile_path = os.path.join(expected_vmdir, 'volatile.img')
-        vm.storage.get_pool(vm.volumes['volatile'])\
+        vm.volumes['volatile'].reset()
-            .reset(vm.volumes['volatile'])
         self.assertEqualAndExists(vm.volumes['volatile'].path,
-                                   expected_volatile_path)
+                                  expected_volatile_path)
 
     def test_013_template_file_images(self):
         """ Check if root.img, private.img, volatile.img and root-cow.img are

qubes/tests/storage_lvm.py

@@ -134,7 +134,7 @@ class TC_00_ThinPool(ThinPoolBase):
         self.assertEqual(volume.name, 'root')
         self.assertEqual(volume.pool, self.pool.name)
         self.assertEqual(volume.size, qubes.config.defaults['root_img_size'])
-        self.pool.create(volume)
+        volume.create()
         path = "/dev/%s" % volume.vid
         self.assertTrue(os.path.exists(path))
         self.pool.remove(volume)
@@ -154,7 +154,7 @@ class TC_00_ThinPool(ThinPoolBase):
         self.assertEqual(volume.name, 'root')
         self.assertEqual(volume.pool, self.pool.name)
         self.assertEqual(volume.size, qubes.config.defaults['root_img_size'])
-        self.pool.create(volume)
+        volume.create()
         path = "/dev/%s" % volume.vid
         self.assertTrue(os.path.exists(path))
         self.pool.remove(volume)

qubes/tests/vm/appvm.py

@@ -79,7 +79,7 @@ class TC_90_AppVM(qubes.tests.vm.qubesvm.QubesVMTestsMixin,
         self.assertFalse(vm.volume_config['root']['save_on_stop'])
         self.assertTrue(vm.volume_config['root']['snap_on_start'])
         self.assertEqual(vm.volume_config['root'].get('source', None),
-            self.template.volumes['root'].source)
+            self.template.volumes['root'])
 
         self.assertFalse(
             vm.volume_config['volatile'].get('save_on_stop', False))

qubes/vm/appvm.py

@@ -98,9 +98,6 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
                         del self.volume_config[name]['vid']
 
         super(AppVM, self).__init__(app, xml, **kwargs)
-        if 'source' not in self.volume_config['root']:
-            msg = 'missing source for root volume'
-            raise qubes.exc.QubesException(msg)
 
     @qubes.events.handler('domain-load')
     def on_domain_loaded(self, event):

qubes/vm/qubesvm.py

@@ -734,8 +734,10 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
         if not hasattr(self, 'uuid'):
             self.uuid = uuid.uuid4()
 
-        # Initialize VM image storage class
+        # Initialize VM image storage class;
-        self.storage = qubes.storage.Storage(self)
+        # it might be already initialized by a recursive call from a child VM
+        if self.storage is None:
+            self.storage = qubes.storage.Storage(self)
 
     @qubes.events.handler('property-set:label')
     def on_property_set_label(self, event, name, newvalue, oldvalue=None):
@@ -1761,15 +1763,15 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
         volume_config['size'] = source.size
         volume_config['pool'] = source.pool
 
-        has_source = (
+        needs_source = (
-            'source' in volume_config and volume_config['source'] is not None)
+            'source' in volume_config)
         is_snapshot = 'snap_on_start' in volume_config and volume_config[
             'snap_on_start']
-        if is_snapshot and not has_source:
+        if is_snapshot and needs_source:
             if source.source is not None:
                 volume_config['source'] = source.source
             else:
-                volume_config['source'] = source.vid
+                volume_config['source'] = source
         return volume_config
 
     def relative_path(self, path):