storage: move volume_config['source'] filling to one place

Don't set 'source' volume in various places (each VM class constructor
etc), do it as part of volume initialization. And when it needs to be
re-calculated, call storage.init_volume again.

This code was duplicated, and as usual in such a case, those copies
were different - one have set 'size', the other one not.

QubesOS/qubes-issues#2256

qubes/storage/__init__.py

@@ -349,34 +349,44 @@ class Storage(object):
 
         if hasattr(vm, 'volume_config'):
             for name, conf in self.vm.volume_config.items():
-                conf = conf.copy()
-                if 'source' in conf:
-                    template = getattr(vm, 'template', None)
-                    # recursively lookup source volume - templates may be
-                    # chained (TemplateVM -> AppVM -> DispVM, where the
-                    # actual source should be used from TemplateVM)
-                    while 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]
-                        if conf['source'].source is not None:
-                            template = getattr(template, 'template', None)
-                        else:
-                            break
-
                 self.init_volume(name, conf)
 
+    def _update_volume_config_source(self, name, volume_config):
+        '''Retrieve 'source' volume from VM's template'''
+        template = getattr(self.vm, 'template', None)
+        # recursively lookup source volume - templates may be
+        # chained (TemplateVM -> AppVM -> DispVM, where the
+        # actual source should be used from TemplateVM)
+        while template:
+            source = template.volumes[name]
+            volume_config['source'] = source
+            volume_config['pool'] = source.pool
+            volume_config['size'] = source.size
+            if source.source is not None:
+                template = getattr(template, 'template', None)
+            else:
+                break
+
     def init_volume(self, name, volume_config):
         ''' Initialize Volume instance attached to this domain '''
 
         if 'name' not in volume_config:
             volume_config['name'] = name
 
+        if 'source' in volume_config:
+            # we have no control over VM load order,
+            # so initialize storage recursively if needed
+            template = getattr(self.vm, 'template', None)
+            if template and template.storage is None:
+                template.storage = Storage(template)
+
+            if volume_config['source'] is None:
+                self._update_volume_config_source(name, volume_config)
+            else:
+                # if source is already specified, pool needs to be too
+                pool = self.vm.app.get_pool(volume_config['pool'])
+                volume_config['source'] = pool.volumes[volume_config['source']]
+
         # if pool still unknown, load default
         if 'pool' not in volume_config:
             volume_config['pool'] = \

qubes/tests/vm/appvm.py

@@ -110,7 +110,7 @@ class TC_90_AppVM(qubes.tests.vm.qubesvm.QubesVMTestsMixin,
         self.assertNotEqual(vm.volume_config['root'].get('source', None),
             self.template.volumes['root'].source)
         self.assertEqual(vm.volume_config['root'].get('source', None),
-            template2.volumes['root'].source)
+            template2.volumes['root'])
 
     def test_003_template_change_running(self):
         vm = self.get_vm()

qubes/vm/appvm.py

@@ -79,12 +79,6 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
             # template is only passed if the AppVM is created, in other cases we
             # don't need to patch the volume_config because the config is
             # coming from XML, already as we need it
-
-            for name, conf in self.volume_config.items():
-                tpl_volume = template.volumes[name]
-
-                self.config_volume_from_source(conf, tpl_volume)
-
             for name, config in template.volume_config.items():
                 # in case the template vm has more volumes add them to own
                 # config
@@ -120,9 +114,5 @@ class AppVM(qubes.vm.qubesvm.QubesVM):
             if conf.get('snap_on_start', False) and \
                     conf.get('source', None) is None:
                 config = conf.copy()
-                template_volume = newvalue.volumes[volume_name]
+                self.volume_config[volume_name] = config
-                self.volume_config[volume_name] = \
-                    self.config_volume_from_source(
-                        config,
-                        template_volume)
                 self.storage.init_volume(volume_name, config)

qubes/vm/dispvm.py

@@ -78,11 +78,6 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
             # template is only passed if the AppVM is created, in other cases we
             # don't need to patch the volume_config because the config is
             # coming from XML, already as we need it
-
-            for name, conf in self.volume_config.items():
-                tpl_volume = template.volumes[name]
-                self.config_volume_from_source(conf, tpl_volume)
-
             for name, config in template.volume_config.items():
                 # in case the template vm has more volumes add them to own
                 # config

qubes/vm/qubesvm.py

@@ -1682,24 +1682,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM):
     # helper methods
     #
 
-    @staticmethod
-    def config_volume_from_source(volume_config, source):
-        '''Adjust storage volume config to use given volume as a source'''
-
-        volume_config['size'] = source.size
-        volume_config['pool'] = source.pool
-
-        needs_source = (
-            'source' in volume_config)
-        is_snapshot = 'snap_on_start' in volume_config and volume_config[
-            'snap_on_start']
-        if is_snapshot and needs_source:
-            if source.source is not None:
-                volume_config['source'] = source.source
-            else:
-                volume_config['source'] = source
-        return volume_config
-
     def relative_path(self, path):
         '''Return path relative to py:attr:`dir_path`.