From c5667791e8f4de847793203c5440f369ee2e4f96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 25 Jul 2017 05:29:26 +0200 Subject: [PATCH] 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 | 50 +++++++++++++++++++++++---------------- qubes/tests/vm/appvm.py | 2 +- qubes/vm/appvm.py | 12 +--------- qubes/vm/dispvm.py | 5 ---- qubes/vm/qubesvm.py | 18 -------------- 5 files changed, 32 insertions(+), 55 deletions(-) diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py index fdfebea0..97e2bda0 100644 --- a/qubes/storage/__init__.py +++ b/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'] = \ diff --git a/qubes/tests/vm/appvm.py b/qubes/tests/vm/appvm.py index 2a4f3f37..ecfb9283 100644 --- a/qubes/tests/vm/appvm.py +++ b/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() diff --git a/qubes/vm/appvm.py b/qubes/vm/appvm.py index ff2db7bb..efaaa23b 100644 --- a/qubes/vm/appvm.py +++ b/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] = \ - self.config_volume_from_source( - config, - template_volume) + self.volume_config[volume_name] = config self.storage.init_volume(volume_name, config) diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py index 183a3f32..b9f6bf39 100644 --- a/qubes/vm/dispvm.py +++ b/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 diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 69febe4a..1d9b7430 100644 --- a/qubes/vm/qubesvm.py +++ b/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`.