dispvm: fix setting up new DispVM

Clone properties from DispVM template after setting base properties
(qid, name, uuid). This means we can use standard clone_properties()
function. Otherwise various setters may fail - for example
netvm setter require uuid property initialized (for VM lookup in VM
collection).
Also, make dispvm_allowed check more robust - include direct creation of
DispVM, and also check just before VM startup (if property was changed
in the meantime).

Fixes QubesOS/qubes-issues#3057

qubes/__init__.py

@@ -640,7 +640,7 @@ class PropertyHolder(qubes.events.Emitter):
         '''Clone properties from other object.
 
         :param PropertyHolder src: source object
-        :param list proplist: list of properties \
+        :param iterable proplist: list of properties \
             (:py:obj:`None` or omit for all properties except those with \
             :py:attr:`property.clone` set to :py:obj:`False`)
         '''

qubes/tests/vm/dispvm.py

@@ -93,3 +93,51 @@ class TC_00_DispVM(qubes.tests.QubesTestCase):
         with self.assertRaises(qubes.exc.QubesException):
             dispvm = self.loop.run_until_complete(
                 qubes.vm.dispvm.DispVM.from_appvm(self.appvm))
+
+    def test_010_create_direct(self):
+        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.app.add_new_vm(qubes.vm.dispvm.DispVM,
+                name='test-dispvm', template=self.appvm)
+            mock_domains.get_new_unused_dispid.assert_called_once_with()
+        self.assertEqual(dispvm.name, 'test-dispvm')
+        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, False)
+
+    def test_011_create_direct_generate_name(self):
+        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.app.add_new_vm(qubes.vm.dispvm.DispVM,
+                template=self.appvm)
+            mock_domains.get_new_unused_dispid.assert_called_once_with()
+        self.assertEqual(dispvm.name, 'disp42')
+        self.assertEqual(dispvm.template, self.appvm)
+        self.assertEqual(dispvm.label, self.appvm.label)
+        self.assertEqual(dispvm.auto_cleanup, False)
+
+    def test_011_create_direct_reject(self):
+        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
+            })
+            with self.assertRaises(qubes.exc.QubesException):
+                self.app.add_new_vm(qubes.vm.dispvm.DispVM,
+                    name='test-dispvm', template=self.appvm)
+            self.assertFalse(mock_domains.get_new_unused_dispid.called)

qubes/vm/dispvm.py

@@ -77,22 +77,18 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
         template = kwargs.get('template', None)
 
         if xml is None:
+            assert template is not None
+
+            if not template.dispvm_allowed:
+                raise qubes.exc.QubesValueError(
+                    'template for DispVM ({}) needs to have '
+                    'dispvm_allowed=True'.format(template.name))
+
             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
@@ -108,6 +104,13 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
         super(DispVM, self).__init__(app, xml, *args, **kwargs)
 
         if xml is None:
+            # 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()]
+            self.clone_properties(template, set(proplist).intersection(
+                self_props))
+
             self.firewall.clone(template.firewall)
             self.features.update(template.features)
             self.tags.update(template.tags)
@@ -189,3 +192,15 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
             yield from self.remove_from_disk()
             del self.app.domains[self]
             self.app.save()
+
+    @asyncio.coroutine
+    def start(self, **kwargs):
+        # pylint: disable=arguments-differ
+
+        # sanity check, if template_for_dispvm got changed in the meantime
+        if not self.template.dispvm_allowed:
+            raise qubes.exc.QubesException(
+                'template for DispVM ({}) needs to have '
+                'dispvm_allowed=True'.format(self.template.name))
+
+        yield from super(DispVM, self).start(**kwargs)