From f6d10ec24365bd2e6a543518e9b77938d3a2e7b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 11 Sep 2017 13:54:32 +0200 Subject: [PATCH 01/15] block: improve handling device name and description Don't fail when device have no description. Also, handle device name consistently - there is already name_re defined. --- qubes/ext/block.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/qubes/ext/block.py b/qubes/ext/block.py index 28c4128f..235c2ae2 100644 --- a/qubes/ext/block.py +++ b/qubes/ext/block.py @@ -58,6 +58,8 @@ class BlockDevice(qubes.devices.DeviceInfo): string.ascii_letters + string.digits + '()+,-.:=_/ '} untrusted_desc = self.backend_domain.untrusted_qdb.read( '/qubes-block-devices/{}/desc'.format(self.ident)) + if not untrusted_desc: + return '' desc = ''.join((chr(c) if c in safe_set else '_') for c in untrusted_desc) self._description = desc @@ -136,14 +138,13 @@ class BlockDeviceExtension(qubes.ext.Extension): def on_device_list_block(self, vm, event): # pylint: disable=unused-argument,no-self-use - safe_set = string.ascii_letters + string.digits if not vm.is_running(): return untrusted_qubes_devices = vm.untrusted_qdb.list('/qubes-block-devices/') untrusted_idents = set(untrusted_path.split('/', 3)[2] for untrusted_path in untrusted_qubes_devices) for untrusted_ident in untrusted_idents: - if not all(c in safe_set for c in untrusted_ident): + if not name_re.match(untrusted_ident): msg = ("%s vm's device path name contains unsafe characters. " "Skipping it.") vm.log.warning(msg % vm.name) From fd5aaa8866891ec4d751a6eaad2361cb29d33199 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 12 Sep 2017 04:16:33 +0200 Subject: [PATCH 02/15] block: fix handling non-existing devices Don't yield None as DeviceInfo object. The device-get: event handlers are expecte to yield anything only when there is a device. --- qubes/ext/block.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/qubes/ext/block.py b/qubes/ext/block.py index 235c2ae2..209fb240 100644 --- a/qubes/ext/block.py +++ b/qubes/ext/block.py @@ -162,7 +162,9 @@ class BlockDeviceExtension(qubes.ext.Extension): if not vm.is_running(): return if not vm.app.vmm.offline_mode: - yield self.device_get(vm, ident) + device_info = self.device_get(vm, ident) + if device_info: + yield device_info @qubes.ext.handler('device-list-attached:block') def on_device_list_attached(self, vm, event, **kwargs): From 3548ee1163c14b2f921ebc8b55ffb390167fbe4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 22 Sep 2017 21:26:49 +0200 Subject: [PATCH 03/15] ext/block: properly list devtype=cdrom option --- qubes/ext/block.py | 2 ++ qubes/tests/devices_block.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/qubes/ext/block.py b/qubes/ext/block.py index 209fb240..38d5ae28 100644 --- a/qubes/ext/block.py +++ b/qubes/ext/block.py @@ -205,6 +205,8 @@ class BlockDeviceExtension(qubes.ext.Extension): else: options['read-only'] = 'no' options['frontend-dev'] = frontend_dev + if disk.get('device') != 'disk': + options['devtype'] = disk.get('device') if dev_path.startswith('/dev/'): ident = dev_path[len('/dev/'):] diff --git a/qubes/tests/devices_block.py b/qubes/tests/devices_block.py index a2e385ae..8adbf806 100644 --- a/qubes/tests/devices_block.py +++ b/qubes/tests/devices_block.py @@ -328,6 +328,29 @@ class TC_00_Block(qubes.tests.QubesTestCase): self.assertEqual(options['frontend-dev'], 'xvdi') self.assertEqual(options['read-only'], 'no') + def test_033_list_attached_cdrom(self): + disk = ''' + + + + + + + + ''' + vm = TestVM({}, domain_xml=domain_xml_template.format(disk)) + vm.app.domains['test-vm'] = vm + vm.app.domains['sys-usb'] = TestVM({}, name='sys-usb') + devices = sorted(list(self.ext.on_device_list_attached(vm, ''))) + self.assertEqual(len(devices), 1) + dev = devices[0][0] + options = devices[0][1] + self.assertEqual(dev.backend_domain, vm.app.domains['sys-usb']) + self.assertEqual(dev.ident, 'sr0') + self.assertEqual(options['frontend-dev'], 'xvdi') + self.assertEqual(options['read-only'], 'yes') + self.assertEqual(options['devtype'], 'cdrom') + def test_040_attach(self): back_vm = TestVM(name='sys-usb', qdb={ '/qubes-block-devices/sda': b'', From 7c6b04677f8be4bd9f99b0574bb5554265744324 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 22 Sep 2017 22:26:32 +0200 Subject: [PATCH 04/15] vm/dispvm: fix error message Fixes QubesOS/qubes-issues#3114 --- qubes/vm/dispvm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py index c70f4449..703b9773 100644 --- a/qubes/vm/dispvm.py +++ b/qubes/vm/dispvm.py @@ -164,7 +164,7 @@ class DispVM(qubes.vm.qubesvm.QubesVM): if not appvm.template_for_dispvms: raise qubes.exc.QubesException( 'Refusing to create DispVM out of this AppVM, because ' - 'template_for_appvms=False') + 'template_for_dispvms=False') app = appvm.app dispvm = app.add_new_vm( cls, From e38e2275039d65876d35e717746fc89b2b17aa6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 22 Sep 2017 22:45:47 +0200 Subject: [PATCH 05/15] vm/dispvm: cleanup DispVM also on failed startup If dispvm.auto_cleanup is set, cleanup it also after failed startup (like not enough memory). Fixes QubesOS/qubes-issues#3045 --- qubes/vm/dispvm.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py index 703b9773..67f13980 100644 --- a/qubes/vm/dispvm.py +++ b/qubes/vm/dispvm.py @@ -137,7 +137,7 @@ class DispVM(qubes.vm.qubesvm.QubesVM): ''' with (yield from self.startup_lock): yield from self.storage.stop() - if self.auto_cleanup: + if self.auto_cleanup and self in self.app.domains: yield from self.remove_from_disk() del self.app.domains[self] self.app.save() @@ -197,10 +197,19 @@ class DispVM(qubes.vm.qubesvm.QubesVM): def start(self, **kwargs): # pylint: disable=arguments-differ - # sanity check, if template_for_dispvm got changed in the meantime - if not self.template.template_for_dispvms: - raise qubes.exc.QubesException( - 'template for DispVM ({}) needs to have ' - 'template_for_dispvms=True'.format(self.template.name)) + try: + # sanity check, if template_for_dispvm got changed in the meantime + if not self.template.template_for_dispvms: + raise qubes.exc.QubesException( + 'template for DispVM ({}) needs to have ' + 'template_for_dispvms=True'.format(self.template.name)) - yield from super(DispVM, self).start(**kwargs) + yield from super(DispVM, self).start(**kwargs) + except: + # cleanup also on failed startup; there is potential race with + # self.on_domain_shutdown_coro, so check if wasn't already removed + if self.auto_cleanup and self in self.app.domains: + yield from self.remove_from_disk() + del self.app.domains[self] + self.app.save() + raise From 8c847faacceb8e990e613ca9c473758727a23d66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 22 Sep 2017 22:47:09 +0200 Subject: [PATCH 06/15] vm/qubesvm: remove duplicated qmemman_client.close() --- qubes/vm/qubesvm.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 1eaf97e0..7ae6077f 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -872,15 +872,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): self.log.warning('Activating the {} VM'.format(self.name)) self.libvirt_domain.resume() - # close() is not really needed, because the descriptor is - # close-on-exec anyway, the reason to postpone close() is that - # possibly xl is not done constructing the domain after its main - # process exits so we close() when we know the domain is up the - # successful unpause is some indicator of it - if qmemman_client: - qmemman_client.close() - qmemman_client = None - yield from self.start_qrexec_daemon() yield from self.fire_event_async('domain-start', @@ -893,9 +884,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): # running or paused yield from self.kill() # pylint: disable=not-an-iterable raise - finally: - if qmemman_client: - qmemman_client.close() return self From efe600537e2f51b96eb1927f675c9b1beb0315c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Fri, 22 Sep 2017 22:47:30 +0200 Subject: [PATCH 07/15] vm/qubesvm: emit event on failed startup If VM startup failed before starting anything (even in paused state), there will be no further event, not even domain-shutdown. This makes it hard for event-listening applications (like domains tray) to account domain state. Fix this by emiting domain-start-failed event in every case of failed startup after emiting domain-pre-start. Related QubesOS/qubes-issues#3100 --- qubes/vm/qubesvm.py | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 7ae6077f..f57dee11 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -839,17 +839,24 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): pre_event=True, start_guid=start_guid, mem_required=mem_required) - yield from self.storage.verify() + try: + yield from self.storage.verify() - if self.netvm is not None: - # pylint: disable = no-member - if self.netvm.qid != 0: - if not self.netvm.is_running(): - yield from self.netvm.start(start_guid=start_guid, - notify_function=notify_function) + if self.netvm is not None: + # pylint: disable = no-member + if self.netvm.qid != 0: + if not self.netvm.is_running(): + yield from self.netvm.start(start_guid=start_guid, + notify_function=notify_function) - qmemman_client = yield from asyncio.get_event_loop().\ - run_in_executor(None, self.request_memory, mem_required) + qmemman_client = yield from asyncio.get_event_loop().\ + run_in_executor(None, self.request_memory, mem_required) + + except Exception as exc: + # let anyone receiving domain-pre-start know that startup failed + yield from self.fire_event_async('domain-start-failed', + reason=str(exc)) + raise try: yield from self.storage.start() @@ -857,6 +864,13 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): self.libvirt_domain.createWithFlags( libvirt.VIR_DOMAIN_START_PAUSED) + + except Exception as exc: + # let anyone receiving domain-pre-start know that startup failed + yield from self.fire_event_async('domain-start-failed', + reason=str(exc)) + raise + finally: if qmemman_client: qmemman_client.close() @@ -877,12 +891,16 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): yield from self.fire_event_async('domain-start', start_guid=start_guid) - except: # pylint: disable=bare-except + except Exception as exc: # pylint: disable=bare-except if self.is_running() or self.is_paused(): # This avoids losing the exception if an exception is # raised in self.force_shutdown(), because the vm is not # running or paused yield from self.kill() # pylint: disable=not-an-iterable + + # let anyone receiving domain-pre-start know that startup failed + yield from self.fire_event_async('domain-start-failed', + reason=str(exc)) raise return self From dc0e1a548170fd916915aac5b585620231a8bc1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sat, 23 Sep 2017 16:02:01 +0200 Subject: [PATCH 08/15] vm: do not allow deleting template property from AppVM and DispVM There is intentionally no default template in terms of qubes.property definition, to not cause problems when switching global default_template property - like breaking some VMs, or forcing the user to shutdown all of them for this. But this also means it shouldn't be allowed to reset template to "default" value, because it will result in a VM without template at all. Fixes QubesOS/qubes-issues#3115 --- qubes/tests/vm/appvm.py | 5 +++++ qubes/tests/vm/dispvm.py | 18 ++++++++++++++++++ qubes/vm/appvm.py | 6 ++++++ qubes/vm/dispvm.py | 5 +++-- 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/qubes/tests/vm/appvm.py b/qubes/tests/vm/appvm.py index 73bfc207..5dfc9c5d 100644 --- a/qubes/tests/vm/appvm.py +++ b/qubes/tests/vm/appvm.py @@ -130,6 +130,11 @@ class TC_90_AppVM(qubes.tests.vm.qubesvm.QubesVMTestsMixin, with self.assertRaises(qubes.exc.QubesVMNotHaltedError): vm.template = self.template + def test_004_template_reset(self): + vm = self.get_vm() + with self.assertRaises(qubes.exc.QubesValueError): + vm.template = qubes.property.DEFAULT + def test_500_property_migrate_template_for_dispvms(self): xml_template = ''' diff --git a/qubes/tests/vm/dispvm.py b/qubes/tests/vm/dispvm.py index f8b3174d..4236d258 100644 --- a/qubes/tests/vm/dispvm.py +++ b/qubes/tests/vm/dispvm.py @@ -103,6 +103,24 @@ class TC_00_DispVM(qubes.tests.QubesTestCase): dispvm = self.loop.run_until_complete( qubes.vm.dispvm.DispVM.from_appvm(self.appvm)) + def test_002_template_change(self): + self.appvm.template_for_dispvms = True + orig_getitem = self.app.domains.__getitem__ + with mock.patch.object(self.app, 'domains', wraps=self.app.domains) \ + as mock_domains: + mock_domains.configure_mock(**{ + 'get_new_unused_dispid': mock.Mock(return_value=42), + '__getitem__.side_effect': orig_getitem + }) + dispvm = self.app.add_new_vm(qubes.vm.dispvm.DispVM, + name='test-dispvm', template=self.appvm) + + with self.assertRaises(qubes.exc.QubesValueError): + dispvm.template = self.appvm + with self.assertRaises(qubes.exc.QubesValueError): + dispvm.template = qubes.property.DEFAULT + + def test_010_create_direct(self): self.appvm.template_for_dispvms = True orig_getitem = self.app.domains.__getitem__ diff --git a/qubes/vm/appvm.py b/qubes/vm/appvm.py index b853265b..8ab808d3 100644 --- a/qubes/vm/appvm.py +++ b/qubes/vm/appvm.py @@ -104,6 +104,12 @@ class AppVM(qubes.vm.qubesvm.QubesVM): ''' # pylint: disable=unused-argument assert self.template + @qubes.events.handler('property-pre-del:template') + def on_property_pre_del_template(self, event, name, oldvalue=None): + '''Forbid deleting template of running VM + ''' # pylint: disable=unused-argument,no-self-use + raise qubes.exc.QubesValueError('Cannot unset template') + @qubes.events.handler('property-pre-set:template') def on_property_pre_set_template(self, event, name, newvalue, oldvalue=None): diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py index 67f13980..fc160b6a 100644 --- a/qubes/vm/dispvm.py +++ b/qubes/vm/dispvm.py @@ -121,8 +121,9 @@ class DispVM(qubes.vm.qubesvm.QubesVM): ''' # pylint: disable=unused-argument assert self.template - @qubes.events.handler('property-pre-set:template') - def on_property_pre_set_template(self, event, name, newvalue, + @qubes.events.handler('property-pre-set:template', + 'property-pre-del:template') + def on_property_pre_set_template(self, event, name, newvalue=None, oldvalue=None): ''' Disposable VM cannot have template changed ''' # pylint: disable=unused-argument From 9f88fa7f0cab4f7ff7e5acb4856dbcbbfe0856e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 26 Sep 2017 13:24:22 +0200 Subject: [PATCH 09/15] Move QubesVM.{name,qid,uuid,label} to BaseVM Reduce strange code in BaseVM (accessing non-existing self.name) and code duplication. --- qubes/tests/vm/qubesvm.py | 6 ++--- qubes/vm/__init__.py | 51 ++++++++++++++++++++++++++++++--------- qubes/vm/adminvm.py | 8 ++---- qubes/vm/qubesvm.py | 35 --------------------------- 4 files changed, 45 insertions(+), 55 deletions(-) diff --git a/qubes/tests/vm/qubesvm.py b/qubes/tests/vm/qubesvm.py index b98363a4..e7c299aa 100644 --- a/qubes/tests/vm/qubesvm.py +++ b/qubes/tests/vm/qubesvm.py @@ -71,15 +71,15 @@ class TC_00_setters(qubes.tests.QubesTestCase): def test_000_setter_qid(self): self.assertEqual( - qubes.vm.qubesvm._setter_qid(self.vm, self.prop, 5), 5) + qubes.vm._setter_qid(self.vm, self.prop, 5), 5) def test_001_setter_qid_lt_0(self): with self.assertRaises(ValueError): - qubes.vm.qubesvm._setter_qid(self.vm, self.prop, -1) + qubes.vm._setter_qid(self.vm, self.prop, -1) def test_002_setter_qid_gt_max(self): with self.assertRaises(ValueError): - qubes.vm.qubesvm._setter_qid(self.vm, + qubes.vm._setter_qid(self.vm, self.prop, qubes.config.max_qid + 5) @unittest.skip('test not implemented') diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index fe2e1288..853d57a7 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -27,6 +27,7 @@ import asyncio import re import string +import uuid import lxml.etree @@ -64,6 +65,26 @@ def validate_name(holder, prop, value): raise qubes.exc.QubesValueError( 'VM name cannot be \'none\' nor \'default\'') +def setter_label(self, prop, value): + ''' Helper for setting the domain label ''' + # pylint: disable=unused-argument + if isinstance(value, qubes.Label): + return value + if isinstance(value, str) and value.startswith('label-'): + return self.app.labels[int(value.split('-', 1)[1])] + + return self.app.get_label(value) + + +def _setter_qid(self, prop, value): + ''' Helper for setting the domain qid ''' + # pylint: disable=unused-argument + value = int(value) + if not 0 <= value <= qubes.config.max_qid: + raise ValueError( + '{} value must be between 0 and qubes.config.max_qid'.format( + prop.__name__)) + return value class Features(dict): '''Manager of the features. @@ -262,6 +283,25 @@ class BaseVM(qubes.PropertyHolder): ''' # pylint: disable=no-member + uuid = qubes.property('uuid', type=uuid.UUID, write_once=True, + clone=False, + doc='UUID from libvirt.') + + name = qubes.property('name', type=str, write_once=True, + clone=False, + doc='User-specified name of the domain.') + + qid = qubes.property('qid', type=int, write_once=True, + setter=_setter_qid, + clone=False, + doc='''Internal, persistent identificator of particular domain. Note + this is different from Xen domid.''') + + label = qubes.property('label', + setter=setter_label, + doc='''Colourful label assigned to VM. This is where the colour of the + padlock is set.''') + def __init__(self, app, xml, features=None, devices=None, tags=None, **kwargs): # pylint: disable=redefined-outer-name @@ -531,14 +571,3 @@ class VMProperty(qubes.property): return untrusted_vmname validate_name(None, self, untrusted_vmname) return untrusted_vmname - - -def setter_label(self, prop, value): - ''' Helper for setting the domain label ''' - # pylint: disable=unused-argument - if isinstance(value, qubes.Label): - return value - if isinstance(value, str) and value.startswith('label-'): - return self.app.labels[int(value.split('-', 1)[1])] - - return self.app.get_label(value) diff --git a/qubes/vm/adminvm.py b/qubes/vm/adminvm.py index 868656fb..dbee80c3 100644 --- a/qubes/vm/adminvm.py +++ b/qubes/vm/adminvm.py @@ -24,10 +24,12 @@ ''' This module contains the AdminVM implementation ''' import libvirt + import qubes import qubes.exc import qubes.vm + class AdminVM(qubes.vm.BaseVM): '''Dom0''' @@ -36,12 +38,6 @@ class AdminVM(qubes.vm.BaseVM): name = qubes.property('name', default='dom0', setter=qubes.property.forbidden) - label = qubes.property('label', - setter=qubes.vm.setter_label, - saver=(lambda self, prop, value: 'label-{}'.format(value.index)), - doc='''Colourful label assigned to VM. This is where the colour of the - padlock is set.''') - qid = qubes.property('qid', default=0, setter=qubes.property.forbidden) diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index f57dee11..67dcabd2 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -59,17 +59,6 @@ MEM_OVERHEAD_BASE = (3 + 1) * 1024 * 1024 MEM_OVERHEAD_PER_VCPU = 3 * 1024 * 1024 / 2 -def _setter_qid(self, prop, value): - ''' Helper for setting the domain qid ''' - # pylint: disable=unused-argument - value = int(value) - if not 0 <= value <= qubes.config.max_qid: - raise ValueError( - '{} value must be between 0 and qubes.config.max_qid'.format( - prop.__name__)) - return value - - def _setter_kernel(self, prop, value): ''' Helper for setting the domain kernel and running sanity checks on it. ''' # pylint: disable=unused-argument @@ -369,30 +358,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): # properties loaded from XML # - label = qubes.property('label', - setter=qubes.vm.setter_label, - saver=(lambda self, prop, value: 'label-{}'.format(value.index)), - doc='''Colourful label assigned to VM. This is where the colour of the - padlock is set.''') - -# provides_network = qubes.property('provides_network', -# type=bool, setter=qubes.property.bool, -# doc='`True` if it is NetVM or ProxyVM, false otherwise.') - - qid = qubes.property('qid', type=int, write_once=True, - setter=_setter_qid, - clone=False, - doc='''Internal, persistent identificator of particular domain. Note - this is different from Xen domid.''') - - name = qubes.property('name', type=str, write_once=True, - clone=False, - doc='User-specified name of the domain.') - - uuid = qubes.property('uuid', type=uuid.UUID, write_once=True, - clone=False, - doc='UUID from libvirt.') - virt_mode = qubes.property('virt_mode', type=str, setter=_setter_virt_mode, default='hvm', From a90dea34de4053355cfd640336bb54a2052dec6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 26 Sep 2017 13:31:12 +0200 Subject: [PATCH 10/15] vm: add vm.klass property Allow to get domain class as a property, not using admin.vm.List call. This makes it unnecessary to call admin.vm.List on the client side to construct wrapper object. --- qubes/vm/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index 853d57a7..02f1d34a 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -500,6 +500,10 @@ class BaseVM(qubes.PropertyHolder): for path in self._qdb_watch_paths: self._qdb_connection_watch.watch(path) + @qubes.stateless_property + def klass(self): + '''Domain class name''' + return type(self).__name__ class VMProperty(qubes.property): '''Property that is referring to a VM From 9e8c40867b3517eecd4493aa5a3ee51dd114b13f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Tue, 26 Sep 2017 14:55:45 +0200 Subject: [PATCH 11/15] Prevent removing domain that is referenced from anywhere Check VM properties and global properties (all of them). Fixes QubesOS/qubes-issues#3128 --- qubes/app.py | 22 +++++---- qubes/exc.py | 5 +++ qubes/tests/app.py | 108 +++++++++++++++++++++++++++++++++++++++------ 3 files changed, 113 insertions(+), 22 deletions(-) diff --git a/qubes/app.py b/qubes/app.py index 72b11e75..47b2d371 100644 --- a/qubes/app.py +++ b/qubes/app.py @@ -35,6 +35,7 @@ import time import traceback import uuid +import itertools import lxml.etree import jinja2 @@ -857,6 +858,8 @@ class Qubes(qubes.PropertyHolder): return element + def __str__(self): + return type(self).__name__ def save(self, lock=True): '''Save all data to qubes.xml @@ -1186,14 +1189,17 @@ class Qubes(qubes.PropertyHolder): @qubes.events.handler('domain-pre-delete') def on_domain_pre_deleted(self, event, vm): # pylint: disable=unused-argument - if isinstance(vm, qubes.vm.templatevm.TemplateVM): - appvms = self.domains.get_vms_based_on(vm) - if appvms: - raise qubes.exc.QubesException( - 'Cannot remove template that has dependent AppVMs. ' - 'Affected are: {}'.format(', '.join( - appvm.name for appvm in sorted(appvms)))) - + for obj in itertools.chain(self.domains, (self,)): + for prop in obj.property_list(): + try: + if isinstance(prop, qubes.vm.VMProperty) and \ + getattr(obj, prop.__name__) == vm: + self.log.error( + 'Cannot remove %s, used by %s.%s', + vm, obj, prop.__name__) + raise qubes.exc.QubesVMInUseError(vm) + except AttributeError: + pass @qubes.events.handler('domain-delete') def on_domain_deleted(self, event, vm): diff --git a/qubes/exc.py b/qubes/exc.py index 8b3fa68c..2dee60a4 100644 --- a/qubes/exc.py +++ b/qubes/exc.py @@ -42,6 +42,11 @@ class QubesVMError(QubesException): super(QubesVMError, self).__init__(msg) self.vm = vm +class QubesVMInUseError(QubesVMError): + '''VM is in use, cannot remove.''' + def __init__(self, vm, msg=None): + super(QubesVMInUseError, self).__init__(vm, + msg or 'Domain is in use: {!r}'.format(vm.name)) class QubesVMNotStartedError(QubesVMError): '''Domain is not started. diff --git a/qubes/tests/app.py b/qubes/tests/app.py index 5ef51f8d..d6a7eda6 100644 --- a/qubes/tests/app.py +++ b/qubes/tests/app.py @@ -271,13 +271,13 @@ class TC_30_VMCollection(qubes.tests.QubesTestCase): # pass -class TC_90_Qubes(qubes.tests.QubesTestCase): +class TC_89_QubesEmpty(qubes.tests.QubesTestCase): def tearDown(self): try: os.unlink('/tmp/qubestest.xml') except: pass - super(TC_90_Qubes, self).tearDown() + super().tearDown() @qubes.tests.skipUnlessDom0 def test_000_init_empty(self): @@ -288,25 +288,105 @@ class TC_90_Qubes(qubes.tests.QubesTestCase): pass qubes.Qubes.create_empty_store('/tmp/qubestest.xml').close() - def test_100_clockvm(self): - app = qubes.Qubes('/tmp/qubestest.xml', load=False, offline_mode=True) - app.load_initial_values() - template = app.add_new_vm('TemplateVM', name='test-template', +class TC_90_Qubes(qubes.tests.QubesTestCase): + def tearDown(self): + try: + os.unlink('/tmp/qubestest.xml') + except: + pass + super().tearDown() + + def setUp(self): + super(TC_90_Qubes, self).setUp() + self.app = qubes.Qubes('/tmp/qubestest.xml', load=False, + offline_mode=True) + self.addCleanup(self.cleanup_qubes) + self.app.load_initial_values() + self.template = self.app.add_new_vm('TemplateVM', name='test-template', label='green') - appvm = app.add_new_vm('AppVM', name='test-vm', template=template, + + def cleanup_qubes(self): + self.app.close() + del self.app + try: + del self.template + except AttributeError: + pass + + def test_100_clockvm(self): + appvm = self.app.add_new_vm('AppVM', name='test-vm', template=self.template, label='red') - self.assertIsNone(app.clockvm) + self.assertIsNone(self.app.clockvm) self.assertNotIn('service.clocksync', appvm.features) - self.assertNotIn('service.clocksync', template.features) - app.clockvm = appvm + self.assertNotIn('service.clocksync', self.template.features) + self.app.clockvm = appvm self.assertIn('service.clocksync', appvm.features) self.assertTrue(appvm.features['service.clocksync']) - app.clockvm = template + self.app.clockvm = self.template self.assertNotIn('service.clocksync', appvm.features) - self.assertIn('service.clocksync', template.features) - self.assertTrue(template.features['service.clocksync']) - app.close() + self.assertIn('service.clocksync', self.template.features) + self.assertTrue(self.template.features['service.clocksync']) + + def test_200_remove_template(self): + appvm = self.app.add_new_vm('AppVM', name='test-vm', + template=self.template, + label='red') + with mock.patch.object(self.app, 'vmm'): + with self.assertRaises(qubes.exc.QubesException): + del self.app.domains[self.template] + + def test_201_remove_netvm(self): + netvm = self.app.add_new_vm('AppVM', name='test-netvm', + template=self.template, provides_network=True, + label='red') + appvm = self.app.add_new_vm('AppVM', name='test-vm', + template=self.template, + label='red') + appvm.netvm = netvm + with mock.patch.object(self.app, 'vmm'): + with self.assertRaises(qubes.exc.QubesVMInUseError): + del self.app.domains[netvm] + + def test_202_remove_default_netvm(self): + netvm = self.app.add_new_vm('AppVM', name='test-netvm', + template=self.template, provides_network=True, + label='red') + self.app.default_netvm = netvm + with mock.patch.object(self.app, 'vmm'): + with self.assertRaises(qubes.exc.QubesVMInUseError): + del self.app.domains[netvm] + + def test_203_remove_default_dispvm(self): + appvm = self.app.add_new_vm('AppVM', name='test-appvm', + template=self.template, + label='red') + self.app.default_dispvm = appvm + with mock.patch.object(self.app, 'vmm'): + with self.assertRaises(qubes.exc.QubesVMInUseError): + del self.app.domains[appvm] + + def test_204_remove_appvm_dispvm(self): + dispvm = self.app.add_new_vm('AppVM', name='test-appvm', + template=self.template, + label='red') + appvm = self.app.add_new_vm('AppVM', name='test-appvm2', + template=self.template, default_dispvm=dispvm, + label='red') + with mock.patch.object(self.app, 'vmm'): + with self.assertRaises(qubes.exc.QubesVMInUseError): + del self.app.domains[dispvm] + + def test_205_remove_appvm_dispvm(self): + appvm = self.app.add_new_vm('AppVM', name='test-appvm', + template=self.template, template_for_dispvms=True, + label='red') + dispvm = self.app.add_new_vm('DispVM', name='test-dispvm', + template=appvm, + label='red') + with mock.patch.object(self.app, 'vmm'): + with self.assertRaises(qubes.exc.QubesVMInUseError): + del self.app.domains[appvm] @qubes.tests.skipUnlessGit def test_900_example_xml_in_doc(self): From 34125d915bdeedcdcfdfa45073b82b7188820091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 28 Sep 2017 02:36:37 +0200 Subject: [PATCH 12/15] storage: fix method name in LinuxModules volume It's `import_volume`, not `clone`. --- qubes/storage/kernels.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qubes/storage/kernels.py b/qubes/storage/kernels.py index b570d38c..aadacd83 100644 --- a/qubes/storage/kernels.py +++ b/qubes/storage/kernels.py @@ -83,8 +83,8 @@ class LinuxModules(Volume): def is_dirty(self): return False - def clone(self, source): - if isinstance(source, LinuxModules): + def import_volume(self, src_volume): + if isinstance(src_volume, LinuxModules): # do nothing return self raise StoragePoolException('clone of LinuxModules volume from ' From 97e3dced92b3a3776ab99e47714601a14e1b9f79 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 28 Sep 2017 02:37:43 +0200 Subject: [PATCH 13/15] vm: move comment Place comment describing self.app near self.app definition. --- qubes/vm/__init__.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index 02f1d34a..069d6f2b 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -306,11 +306,12 @@ class BaseVM(qubes.PropertyHolder): **kwargs): # pylint: disable=redefined-outer-name + self._qdb_watch_paths = set() + self._qdb_connection_watch = None + # self.app must be set before super().__init__, because some property # setters need working .app attribute #: mother :py:class:`qubes.Qubes` object - self._qdb_watch_paths = set() - self._qdb_connection_watch = None self.app = app super(BaseVM, self).__init__(xml, **kwargs) From b12fa13f06e425905c9e3b4534357877f868ea2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 28 Sep 2017 02:38:28 +0200 Subject: [PATCH 14/15] vm: report storage.stop() errors to log Catch exception there and log it. Otherwise asyncio complains about not retrieved exception. There is no one else to handle this exception, because shutdown event is triggered from libvirt, not any Admin API. --- qubes/vm/qubesvm.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index 67dcabd2..c187c65d 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -876,7 +876,11 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): Do not allow domain to be started again until this finishes. ''' with (yield from self.startup_lock): - yield from self.storage.stop() + try: + yield from self.storage.stop() + except qubes.storage.StoragePoolException: + self.log.exception('Failed to stop storage for domain %s', + self.name) @qubes.events.handler('domain-shutdown') def on_domain_shutdown(self, _event, **_kwargs): From 12b7e22d2754fcc3c5c1c8f1ce11e4c55ca60cca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Thu, 28 Sep 2017 16:12:05 +0200 Subject: [PATCH 15/15] vm: do not start QubesDB watch instance multiple times vm.create_qdb_entries can be called multiple times - for example when changing VM IP. Move starting qdb watcher to start(). And just in case, cleanup old watcher (if still exists) before starting new one. This fixes one FD leak. --- qubes/tests/vm/adminvm.py | 2 +- qubes/vm/__init__.py | 10 ++++++++-- qubes/vm/adminvm.py | 2 +- qubes/vm/qubesvm.py | 5 ++--- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/qubes/tests/vm/adminvm.py b/qubes/tests/vm/adminvm.py index 0560e130..6b70ac48 100644 --- a/qubes/tests/vm/adminvm.py +++ b/qubes/tests/vm/adminvm.py @@ -38,7 +38,7 @@ class TC_00_AdminVM(qubes.tests.QubesTestCase): qubes.vm.adminvm.AdminVM, 'start_qdb_watch') as mock_qdb: self.vm = qubes.vm.adminvm.AdminVM(self.app, xml=None) - mock_qdb.assert_called_once_with('dom0') + mock_qdb.assert_called_once_with() self.addCleanup(self.cleanup_adminvm) except: # pylint: disable=bare-except if self.id().endswith('.test_000_init'): diff --git a/qubes/vm/__init__.py b/qubes/vm/__init__.py index 069d6f2b..2c534773 100644 --- a/qubes/vm/__init__.py +++ b/qubes/vm/__init__.py @@ -486,14 +486,20 @@ class BaseVM(qubes.PropertyHolder): self._qdb_connection_watch.close() self._qdb_connection_watch = None - def start_qdb_watch(self, name, loop=None): + def start_qdb_watch(self, loop=None): '''Start watching QubesDB Calling this method in appropriate time is responsibility of child class. ''' + # cleanup old watch connection first, if any + if self._qdb_connection_watch is not None: + asyncio.get_event_loop().remove_reader( + self._qdb_connection_watch.watch_fd()) + self._qdb_connection_watch.close() + import qubesdb # pylint: disable=import-error - self._qdb_connection_watch = qubesdb.QubesDB(name) + self._qdb_connection_watch = qubesdb.QubesDB(self.name) if loop is None: loop = asyncio.get_event_loop() loop.add_reader(self._qdb_connection_watch.watch_fd(), diff --git a/qubes/vm/adminvm.py b/qubes/vm/adminvm.py index dbee80c3..40566c9d 100644 --- a/qubes/vm/adminvm.py +++ b/qubes/vm/adminvm.py @@ -58,7 +58,7 @@ class AdminVM(qubes.vm.BaseVM): self._libvirt_domain = None if not self.app.vmm.offline_mode: - self.start_qdb_watch('dom0') + self.start_qdb_watch() def __str__(self): return self.name diff --git a/qubes/vm/qubesvm.py b/qubes/vm/qubesvm.py index c187c65d..c2c660e0 100644 --- a/qubes/vm/qubesvm.py +++ b/qubes/vm/qubesvm.py @@ -712,7 +712,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): self.storage = qubes.storage.Storage(self) if not self.app.vmm.offline_mode and self.is_running(): - self.start_qdb_watch(self.name) + self.start_qdb_watch() @qubes.events.handler('property-set:label') def on_property_set_label(self, event, name, newvalue, oldvalue=None): @@ -847,6 +847,7 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): self.log.info('Setting Qubes DB info for the VM') yield from self.start_qubesdb() self.create_qdb_entries() + self.start_qdb_watch() self.log.warning('Activating the {} VM'.format(self.name)) self.libvirt_domain.resume() @@ -1742,8 +1743,6 @@ class QubesVM(qubes.vm.mix.net.NetVMMixin, qubes.vm.BaseVM): self.fire_event('domain-qdb-create') - self.start_qdb_watch(self.name) - # TODO async; update this in constructor def _update_libvirt_domain(self): '''Re-initialise :py:attr:`libvirt_domain`.'''