vm/dispvm: add auto_cleanup property, unify creating new DispVM

Add auto_cleanup property, which remove DispVM after its shutdown
- this is to unify DispVM handling - less places needing special
handling after DispVM shutdown.

New DispVM inherit all settings from respective AppVM. Move this from
classmethod `DispVM.from_appvm()`, to DispVM constructor. This unify
creating new DispVM with any other VM class.
Notable exception are attached devices - because only one running VM can
have a device attached, this would prevent second DispVM started from
the same AppVM. If one need DispVM with some device attached, one can
create DispVM with auto_cleanup=False. Such DispVM will still not have
persistent storage (as any other DispVM).

Tests included.

QubesOS/qubes-issues#2974

qubes/tests/__init__.py

@@ -1003,6 +1003,7 @@ def load_tests(loader, tests, pattern): # pylint: disable=unused-argument
             'qubes.tests.vm.mix.net',
             'qubes.tests.vm.adminvm',
             'qubes.tests.vm.appvm',
+            'qubes.tests.vm.dispvm',
             'qubes.tests.app',
             'qubes.tests.tarwriter',
             'qubes.tests.api',

qubes/tests/vm/dispvm.py

@@ -0,0 +1,95 @@
+# -*- encoding: utf-8 -*-
+#
+# The Qubes OS Project, http://www.qubes-os.org
+#
+# Copyright (C) 2017 Marek Marczykowski-Górecki
+#                               <marmarek@invisiblethingslab.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, see <http://www.gnu.org/licenses/>.
+
+import unittest.mock as mock
+
+import asyncio
+
+import qubes.vm.dispvm
+import qubes.vm.appvm
+import qubes.vm.templatevm
+import qubes.tests
+import qubes.tests.vm
+import qubes.tests.vm.appvm
+
+class TestApp(qubes.tests.vm.TestApp):
+    def __init__(self):
+        super(TestApp, self).__init__()
+        self.qid_counter = 1
+
+    def add_new_vm(self, cls, **kwargs):
+        qid = self.qid_counter
+        self.qid_counter += 1
+        vm = cls(self, None, qid=qid, **kwargs)
+        self.domains[vm.name] = vm
+        self.domains[vm] = vm
+        return vm
+
+class TC_00_DispVM(qubes.tests.QubesTestCase):
+    def setUp(self):
+        super(TC_00_DispVM, self).setUp()
+        self.app = TestApp()
+        self.app.save = mock.Mock()
+        self.app.pools['default'] = qubes.tests.vm.appvm.TestPool('default')
+        self.app.pools['linux-kernel'] = mock.Mock(**{
+            'init_volume.return_value.pool': 'linux-kernel'})
+        self.app.vmm.offline_mode = True
+        self.template = self.app.add_new_vm(qubes.vm.templatevm.TemplateVM,
+            name='test-template', label='red')
+        self.appvm = self.app.add_new_vm(qubes.vm.appvm.AppVM,
+            name='test-vm', template=self.template, label='red')
+        self.app.domains[self.appvm.name] = self.appvm
+        self.app.domains[self.appvm] = self.appvm
+
+    @asyncio.coroutine
+    def mock_coro(self, *args, **kwargs):
+        pass
+
+    @mock.patch('os.symlink')
+    @mock.patch('os.makedirs')
+    @mock.patch('qubes.storage.Storage')
+    def test_000_from_appvm(self, mock_storage, mock_makedirs, mock_symlink):
+        mock_storage.return_value.create.side_effect = self.mock_coro
+        self.appvm.dispvm_allowed = 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.loop.run_until_complete(
+                qubes.vm.dispvm.DispVM.from_appvm(self.appvm))
+            mock_domains.get_new_unused_dispid.assert_called_once_with()
+        self.assertTrue(dispvm.name.startswith('disp'))
+        self.assertEqual(dispvm.template, self.appvm)
+        self.assertEqual(dispvm.label, self.appvm.label)
+        self.assertEqual(dispvm.label, self.appvm.label)
+        self.assertEqual(dispvm.auto_cleanup, True)
+        mock_makedirs.assert_called_once_with(
+            '/var/lib/qubes/appvms/' + dispvm.name, mode=0o775)
+        mock_symlink.assert_called_once_with(
+            '/usr/share/icons/hicolor/128x128/devices/appvm-red.png',
+            '/var/lib/qubes/appvms/{}/icon.png'.format(dispvm.name))
+
+    def test_001_from_appvm_reject_not_allowed(self):
+        with self.assertRaises(qubes.exc.QubesException):
+            dispvm = self.loop.run_until_complete(
+                qubes.vm.dispvm.DispVM.from_appvm(self.appvm))

qubes/vm/dispvm.py

@@ -39,7 +39,10 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
         clone=False,
         doc='''Internal, persistent identifier of particular DispVM.''')
 
-    def __init__(self, *args, **kwargs):
+    auto_cleanup = qubes.property('auto_cleanup', type=bool, default=False,
+        doc='automatically remove this VM upon shutdown')
+
+    def __init__(self, app, xml, *args, **kwargs):
         self.volume_config = {
             'root': {
                 'name': 'root',
@@ -70,10 +73,26 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
                 'rw': False,
             }
         }
-        if 'name' not in kwargs and 'dispid' in kwargs:
+
-            kwargs['name'] = 'disp' + str(kwargs['dispid'])
         template = kwargs.get('template', None)
 
+        if xml is None:
+            if 'dispid' not in kwargs:
+                kwargs['dispid'] = app.domains.get_new_unused_dispid()
+            if 'name' not in kwargs:
+                kwargs['name'] = 'disp' + str(kwargs['dispid'])
+
+            # by default inherit properties from the DispVM template
+            proplist = [prop.__name__ for prop in template.property_list()
+                if prop.clone and prop.__name__ not in ['template']]
+            self_props = [prop.__name__ for prop in self.property_list()]
+            for prop in proplist:
+                if prop not in self_props:
+                    continue
+                if prop not in kwargs and \
+                        not template.property_is_default(prop):
+                    kwargs[prop] = getattr(template, prop)
+
         if template is not None:
             # 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
@@ -86,11 +105,12 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
                     if 'vid' in self.volume_config[name]:
                         del self.volume_config[name]['vid']
 
-            # by default inherit label from the DispVM template
+        super(DispVM, self).__init__(app, xml, *args, **kwargs)
-            if 'label' not in kwargs:
-                kwargs['label'] = template.label
 
-        super(DispVM, self).__init__(*args, **kwargs)
+        if xml is None:
+            self.firewall.clone(template.firewall)
+            self.features.update(template.features)
+            self.tags.update(template.tags)
 
     @qubes.events.handler('domain-load')
     def on_domain_loaded(self, event):
@@ -106,6 +126,19 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
         raise qubes.exc.QubesValueError(self,
             'Cannot change template of Disposable VM')
 
+    @asyncio.coroutine
+    def on_domain_shutdown_coro(self):
+        '''Coroutine for executing cleanup after domain shutdown.
+
+        This override default action defined in QubesVM.on_domain_shutdown_coro
+        '''
+        with (yield from self.startup_lock):
+            yield from self.storage.stop()
+            if self.auto_cleanup:
+                yield from self.remove_from_disk()
+                del self.app.domains[self]
+                self.app.save()
+
     @classmethod
     @asyncio.coroutine
     def from_appvm(cls, appvm, **kwargs):
@@ -127,18 +160,14 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
         '''
         if not appvm.dispvm_allowed:
             raise qubes.exc.QubesException(
-                'Refusing to start DispVM out of this AppVM, because '
+                'Refusing to create DispVM out of this AppVM, because '
                 'dispvm_allowed=False')
         app = appvm.app
         dispvm = app.add_new_vm(
             cls,
-            dispid=app.domains.get_new_unused_dispid(),
+            template=appvm,
-            template=app.domains[appvm],
+            auto_cleanup=True,
             **kwargs)
-        # exclude template
-        proplist = [prop for prop in dispvm.property_list()
-            if prop.clone and prop.__name__ not in ['template']]
-        dispvm.clone_properties(app.domains[appvm], proplist=proplist)
         yield from dispvm.create_on_disk()
         app.save()
         return dispvm
@@ -155,6 +184,8 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
             yield from self.kill()
         except qubes.exc.QubesVMNotStartedError:
             pass
-        yield from self.remove_from_disk()
+        # if auto_cleanup is set, this will be done automatically
-        del self.app.domains[self]
+        if not self.auto_cleanup:
-        self.app.save()
+            yield from self.remove_from_disk()
+            del self.app.domains[self]
+            self.app.save()

rpm_spec/core-dom0.spec

@@ -326,6 +326,7 @@ fi
 %{python3_sitelib}/qubes/tests/vm/init.py
 %{python3_sitelib}/qubes/tests/vm/adminvm.py
 %{python3_sitelib}/qubes/tests/vm/appvm.py
+%{python3_sitelib}/qubes/tests/vm/dispvm.py
 %{python3_sitelib}/qubes/tests/vm/qubesvm.py
 
 %dir %{python3_sitelib}/qubes/tests/vm/mix