diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py index f713434d..0517feb3 100644 --- a/qubes/storage/__init__.py +++ b/qubes/storage/__init__.py @@ -275,6 +275,9 @@ class Volume(object): def verify(self): ''' Verifies the volume. + This function is supposed to either return :py:obj:`True`, or raise + an exception. + This can be implemented as a coroutine.''' raise self._not_implemented("verify") diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py index d20d2768..107e7e37 100644 --- a/qubes/storage/lvm.py +++ b/qubes/storage/lvm.py @@ -270,10 +270,22 @@ class ThinVolume(qubes.storage.Volume): qubes_lvm(cmd, self.log) self._remove_revisions() - cmd = ['remove', self.vid] - qubes_lvm(cmd, self.log) - cmd = ['clone', self._vid_snap, self.vid] + # TODO: when converting this function to coroutine, this _must_ be + # under a lock + # remove old volume only after _successful_ clone of the new one + cmd = ['rename', self.vid, self.vid + '-tmp'] qubes_lvm(cmd, self.log) + try: + cmd = ['clone', self._vid_snap, self.vid] + qubes_lvm(cmd, self.log) + except: + # restore original volume + cmd = ['rename', self.vid + '-tmp', self.vid] + qubes_lvm(cmd, self.log) + raise + else: + cmd = ['remove', self.vid + '-tmp'] + qubes_lvm(cmd, self.log) def create(self): @@ -419,34 +431,47 @@ class ThinVolume(qubes.storage.Volume): def start(self): - if self.snap_on_start or self.save_on_stop: - if not self.save_on_stop or not self.is_dirty(): - self._snapshot() - else: - self._reset() - - reset_cache() + try: + if self.snap_on_start or self.save_on_stop: + if not self.save_on_stop or not self.is_dirty(): + self._snapshot() + else: + self._reset() + finally: + reset_cache() return self def stop(self): - if self.save_on_stop: - self._commit() - if self.snap_on_start or self.save_on_stop: - cmd = ['remove', self._vid_snap] - qubes_lvm(cmd, self.log) - else: - cmd = ['remove', self.vid] - qubes_lvm(cmd, self.log) - reset_cache() + try: + if self.save_on_stop: + self._commit() + if self.snap_on_start or self.save_on_stop: + cmd = ['remove', self._vid_snap] + qubes_lvm(cmd, self.log) + else: + cmd = ['remove', self.vid] + qubes_lvm(cmd, self.log) + finally: + reset_cache() return self def verify(self): ''' Verifies the volume. ''' + if not self.save_on_stop and not self.snap_on_start: + # volatile volumes don't need any files + return True + if self.source is not None: + vid = str(self.source) + else: + vid = self.vid try: - vol_info = size_cache[self.vid] - return vol_info['attr'][4] == 'a' + vol_info = size_cache[vid] + if vol_info['attr'][4] != 'a': + raise qubes.storage.StoragePoolException( + 'volume {} not active'.format(vid)) except KeyError: - return False + raise qubes.storage.StoragePoolException( + 'volume {} missing'.format(vid)) def block_device(self): @@ -493,6 +518,8 @@ def qubes_lvm(cmd, log=logging.getLogger('qubes.storage.lvm')): lvm_cmd = ["lvextend", "-L%s" % size, cmd[1]] elif action == 'activate': lvm_cmd = ['lvchange', '-ay', cmd[1]] + elif action == 'rename': + lvm_cmd = ['lvrename', cmd[1], cmd[2]] else: raise NotImplementedError('unsupported action: ' + action) if lvm_is_very_old: diff --git a/qubes/tests/integ/basic.py b/qubes/tests/integ/basic.py index b5bdb476..e939aab9 100644 --- a/qubes/tests/integ/basic.py +++ b/qubes/tests/integ/basic.py @@ -30,9 +30,12 @@ import tempfile import time import unittest +import collections + import qubes import qubes.firewall import qubes.tests +import qubes.storage import qubes.vm.appvm import qubes.vm.qubesvm import qubes.vm.standalonevm @@ -79,6 +82,49 @@ class TC_00_Basic(qubes.tests.SystemTestCase): self.loop.run_until_complete(asyncio.sleep(0.1)) self.assertTrue(flag) + def _test_200_on_domain_start(self, vm, event, **_kwargs): + '''Simulate domain crash just after startup''' + vm.libvirt_domain.destroy() + + def test_200_shutdown_event_race(self): + '''Regression test for 3164''' + vmname = self.make_vm_name('appvm') + + self.vm = self.app.add_new_vm(qubes.vm.appvm.AppVM, + name=vmname, template=self.app.default_template, + label='red') + # help the luck a little - don't wait for qrexec to easier win the race + self.vm.features['qrexec'] = False + self.loop.run_until_complete(self.vm.create_on_disk()) + # another way to help the luck a little - make sure the private + # volume is first in (normally unordered) dict - this way if any + # volume action fails, it will be at or after private volume - not + # before (preventing private volume action) + old_volumes = self.vm.volumes + self.vm.volumes = collections.OrderedDict() + self.vm.volumes['private'] = old_volumes.pop('private') + self.vm.volumes.update(old_volumes.items()) + del old_volumes + + self.loop.run_until_complete(self.vm.start()) + + # kill it the way it does not give a chance for domain-shutdown it + # execute + self.vm.libvirt_domain.destroy() + + # now, lets try to start the VM again, before domain-shutdown event + # got handled (#3164), and immediately trigger second domain-shutdown + self.vm.add_handler('domain-start', self._test_200_on_domain_start) + self.loop.run_until_complete(self.vm.start()) + + # and give a chance for both domain-shutdown handlers to execute + self.loop.run_until_complete(asyncio.sleep(1)) + with self.assertNotRaises(qubes.exc.QubesException): + # if the above caused two domain-shutdown handlers being called + # one after another, private volume is gone + self.loop.run_until_complete(self.vm.storage.verify()) + + class TC_01_Properties(qubes.tests.SystemTestCase): # pylint: disable=attribute-defined-outside-init def setUp(self):