Rename disp template if only preloads are running

Fixes: QubesOS/qubes-issues#10227
For: QubesOS/qubes-issues#1512

Author: ben-grande

qubesmanager/qube_manager.py

@@ -1472,7 +1472,11 @@ def action_removevm_triggered(self):
         for vm_info in self.get_selected_vms():
             vm = vm_info.vm
 
-            dependencies = utils.vm_dependencies(self.qubes_app, vm)
+            dependencies = [
+                (vm, prop)
+                for (vm, prop) in utils.vm_dependencies(self.qubes_app, vm)
+                if vm and not manager_utils.is_preload(vm)
+            ]
 
             if dependencies:
                 list_deps = manager_utils.format_dependencies_list(dependencies)

qubesmanager/settings.py

@@ -87,6 +87,8 @@ def run(self):
                     if holder is None:
                         setattr(self.vm.app, prop, new_vm)
                     else:
+                        if utils.is_preload(holder):
+                            continue
                         setattr(holder, prop, new_vm)
                 except qubesadmin.exc.QubesException:
                     failed_props += [(holder, prop)]
@@ -872,7 +874,10 @@ def rename_vm(self):
         running_dependencies = [
             vm.name
             for (vm, prop) in dependencies
-            if vm and prop == "template" and utils.is_running(vm, False)
+            if vm
+            and prop == "template"
+            and utils.is_running(vm, False)
+            and not utils.is_preload(vm)
         ]
 
         if running_dependencies:
@@ -911,7 +916,11 @@ def rename_vm(self):
 
     def remove_vm(self):
 
-        dependencies = admin_utils.vm_dependencies(self.vm.app, self.vm)
+        dependencies = [
+            (vm, prop)
+            for (vm, prop) in admin_utils.vm_dependencies(self.vm.app, self.vm)
+            if vm and not utils.is_preload(vm)
+        ]
 
         if dependencies:
             list_text = utils.format_dependencies_list(dependencies)
@@ -1239,8 +1248,8 @@ def __apply_advanced_tab__(self):
                 self.dvm_template_checkbox.isChecked()
                 and self.preload_dispvm.isEnabled()
             ):
-                curr_preload_dispvm = (
-                    int(self.vm.features.get("preload-dispvm-max") or 0)
+                curr_preload_dispvm = int(
+                    self.vm.features.get("preload-dispvm-max") or 0
                 )
                 preload_dispvm = self.preload_dispvm.value()
                 if preload_dispvm != curr_preload_dispvm:

qubesmanager/tests/test_qube_manager.py

@@ -489,12 +489,14 @@ def test_212_remove_vm_dependencies(mock_msgbox, qubes_manager):
 @mock.patch("PyQt6.QtWidgets.QInputDialog.getText")
 def test_213_remove_vm_no_dependencies(mock_input, mock_warning, qubes_manager):
     # get a non-running vm
-    _select_vm(qubes_manager, 'test-red')
+    qube_name = 'default-dvm'
+    _select_vm(qubes_manager, qube_name)
+    assert qubes_manager.qubes_app.domains['test-disp'].template.name == qube_name
 
-    with (mock.patch('qubesmanager.common_threads.RemoveVMThread') as
-          mock_thread):
+    with mock.patch('qubesmanager.common_threads.RemoveVMThread') as mock_thread, \
+        mock.patch('qubesmanager.utils.is_preload', return_value=True):
         # user cancels
-        mock_input.return_value = ('test-red', False)
+        mock_input.return_value = (qube_name, False)
         qubes_manager.action_removevm.trigger()
         assert mock_thread.call_count == 0
         assert mock_warning.call_count == 0
@@ -504,11 +506,11 @@ def test_213_remove_vm_no_dependencies(mock_input, mock_warning, qubes_manager):
         assert mock_warning.call_count == 1
         assert mock_thread.call_count == 0
 
-        mock_input.return_value = ('test-red', True)
+        mock_input.return_value = (qube_name, True)
         qubes_manager.action_removevm.trigger()
         assert mock_warning.call_count == 1
         mock_thread.assert_called_once_with(
-            qubes_manager.qubes_app.domains['test-red'])
+            qubes_manager.qubes_app.domains[qube_name])
         mock_thread().finished.connect.assert_called_once_with(
             qubes_manager.clear_threads)
         mock_thread().start.assert_called_once_with()

qubesmanager/tests/test_vm_settings.py

@@ -26,6 +26,8 @@
 
 from PyQt6 import QtCore, QtWidgets
 import qubesmanager.settings as vm_settings
+import qubesmanager.utils as utils
+import qubesadmin.utils as admin_utils
 from qubesadmin.tests.mock_app import MockQube, MockDevice
 
 PAGES = ["basic", "advanced", "firewall", "devices", "applications", "services"]
@@ -38,6 +40,10 @@
     "test-standalone",
     "test-old",
     "test-vm-set",
+    "default-dvm",
+    "test-disp",
+    "test-alt-dvm",
+    "test-alt-disp",
 ]
 
 # with a template
@@ -372,7 +378,9 @@ def test_101_change_template(settings_fixture):
     settings_window, page, vm_name = settings_fixture
     vm = settings_window.qubesapp.domains[vm_name]
 
-    if vm.is_running() or not hasattr(vm, "template"):
+    if vm.klass == "DispVM":
+        return
+    if vm.is_running() or not hasattr(vm, "template") or vm.klass == "StandaloneVM":
         assert not settings_window.template_name.isEnabled()
         return
 
@@ -609,7 +617,9 @@ def test_108_disk_space(settings_fixture):
 @mock.patch("qubesmanager.settings.RenameVMThread")
 @mock.patch("PyQt6.QtWidgets.QMessageBox.warning")
 @pytest.mark.parametrize(
-    "settings_fixture", ["fedora-36", "test-vm-set", "test-blue"], indirect=True
+    "settings_fixture",
+    ["fedora-36", "test-vm-set", "test-blue", "default-dvm", "test-disp"],
+    indirect=True,
 )
 def test_109_renamevm(mock_warning, mock_thread, mock_input, settings_fixture):
     settings_window, page, vm_name = settings_fixture
@@ -619,29 +629,68 @@ def test_109_renamevm(mock_warning, mock_thread, mock_input, settings_fixture):
         assert not settings_window.rename_vm_button.isEnabled()
         assert mock_warning.call_count == 0
         return
-    else:
-        assert settings_window.rename_vm_button.isEnabled()
+    assert settings_window.rename_vm_button.isEnabled()
 
     mock_input.return_value = ("renamed-vm", True)
-    settings_window.rename_vm_button.click()
+    # Qubes with same names from mock_app may be on the system, such as
+    # sys-net, which affects test.
+    dependencies = admin_utils.vm_dependencies(settings_window.qubesapp, vm)
+    running_dependencies = []
+    running_dependencies.extend(
+        [
+            vm
+            for (vm, prop) in dependencies
+            if vm and prop == "template" and utils.is_running(vm, False)
+        ]
+    )
+    for qube in running_dependencies:
+        expected_call = (
+            qube.name,
+            "admin.vm.property.Get",
+            "is_preload",
+            None,
+        )
+        assert expected_call not in settings_window.qubesapp.actual_calls
+        settings_window.qubesapp.expected_calls[expected_call] = (
+            b"0\x00default=False type=bool False"
+        )
+
+    if vm.name == "default-dvm":
+        dispvm = settings_window.qubesapp.domains["test-disp"]
+        expected_call = (
+            dispvm.name,
+            "admin.vm.property.Get",
+            "is_preload",
+            None,
+        )
+        settings_window.qubesapp.expected_calls[expected_call] = (
+            b"0\x00default=False type=bool True"
+        )
+        assert expected_call not in settings_window.qubesapp.actual_calls
+        with mock.patch.object(dispvm, "is_running", return_value=True):
+            settings_window.rename_vm_button.click()
+        assert expected_call in settings_window.qubesapp.actual_calls
+    else:
+        settings_window.rename_vm_button.click()
 
     if vm.name == "fedora-36":
         assert mock_warning.call_count == 1
         assert mock_thread.call_count == 0
         return
-    elif vm.name == "test-vm-set":
-        mock_thread.assert_called_with(vm, "renamed-vm", mock.ANY)
-        mock_thread().start.assert_called_with()
-        assert mock_warning.call_count == 0
 
     assert mock_warning.call_count == 0
+    if vm.name in ["test-vm-set", "default-dvm"]:
+        mock_thread.assert_called_with(vm, "renamed-vm", mock.ANY)
+        mock_thread().start.assert_called_with()
 
 
 @mock.patch("PyQt6.QtWidgets.QInputDialog.getText")
 @mock.patch("qubesmanager.common_threads.RemoveVMThread")
 @mock.patch("PyQt6.QtWidgets.QMessageBox.warning")
 @pytest.mark.parametrize(
-    "settings_fixture", ["fedora-36", "test-vm-set", "test-blue"], indirect=True
+    "settings_fixture",
+    ["fedora-36", "test-vm-set", "test-blue", "default-dvm", "test-alt-dvm"],
+    indirect=True,
 )
 def test_110_deletevm(mock_warning, mock_thread, mock_input, settings_fixture):
     settings_window, page, vm_name = settings_fixture
@@ -651,22 +700,60 @@ def test_110_deletevm(mock_warning, mock_thread, mock_input, settings_fixture):
         assert not settings_window.delete_vm_button.isEnabled()
         assert mock_warning.call_count == 0
         return
-    else:
-        assert settings_window.delete_vm_button.isEnabled()
+    assert settings_window.delete_vm_button.isEnabled()
 
     mock_input.return_value = (vm.name, True)
-    settings_window.delete_vm_button.click()
 
-    if vm.name == "fedora-36":
+    # Qubes with same names from mock_app may be on the system, such as
+    # sys-net, which affects test.
+    dependencies = [
+        (vm, prop)
+        for (vm, prop) in admin_utils.vm_dependencies(settings_window.qubesapp, vm)
+        if vm
+    ]
+    for qube, prop in dependencies:
+        expected_call = (
+            qube.name,
+            "admin.vm.property.Get",
+            "is_preload",
+            None,
+        )
+        assert expected_call not in settings_window.qubesapp.actual_calls
+        settings_window.qubesapp.expected_calls[expected_call] = (
+            b"0\x00default=False type=bool False"
+        )
+
+    if vm.name in ["default-dvm", "test-alt-dvm"]:
+        if vm.name == "default-dvm":
+            dispvm_name = "test-disp"
+        else:
+            dispvm_name = "test-alt-disp"
+        dispvm = settings_window.qubesapp.domains[dispvm_name]
+        expected_call = (
+            dispvm.name,
+            "admin.vm.property.Get",
+            "is_preload",
+            None,
+        )
+        settings_window.qubesapp.expected_calls[expected_call] = (
+            b"0\x00default=False type=bool True"
+        )
+        assert expected_call not in settings_window.qubesapp.actual_calls
+        with mock.patch.object(dispvm, "is_running", return_value=True):
+            settings_window.delete_vm_button.click()
+        assert expected_call in settings_window.qubesapp.actual_calls
+    else:
+        settings_window.delete_vm_button.click()
+
+    if vm.name in ["fedora-36", "default-dvm"]:
         assert mock_warning.call_count == 1
         assert mock_thread.call_count == 0
         return
-    elif vm.name == "test-vm-set":
-        mock_thread.assert_called_with(vm)
-        mock_thread().start.assert_called_with()
-        assert mock_warning.call_count == 0
 
     assert mock_warning.call_count == 0
+    if vm.name in ["test-vm-set", "test-alt-dvm"]:
+        mock_thread.assert_called_with(vm)
+        mock_thread().start.assert_called_with()
 
 
 @mock.patch("PyQt6.QtWidgets.QInputDialog.getText")

qubesmanager/utils.py

@@ -85,13 +85,22 @@ def is_internal(vm):
 
 def is_running(vm, default_state):
     """Checks if the VM is running, returns default_state if we have
-    insufficient permissions to deteremine that."""
+    insufficient permissions to determine that."""
     try:
         return vm.is_running()
     except exc.QubesDaemonAccessError:
         return default_state
 
 
+def is_preload(vm):
+    """Checks if the VM is a preloaded disposable, returns False if we have
+    insufficient permissions to determine that."""
+    try:
+        return getattr(vm, "is_preload", False)
+    except exc.QubesDaemonAccessError:
+        return False
+
+
 def translate(string):
     """helper function for translations"""
     return QtCore.QCoreApplication.translate(