Skip to content

Rename disp template if only preloads are running #429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Rename disp template if only preloads are running #429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion qubesmanager/qube_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 not vm or (vm and not manager_utils.is_preload(vm))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any situation in which a preloaded VM should be another VM's dependency? It feels to me like they should just never be returned by the vm_dependencies function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, there is no such situation, but if the user chooses to make it, we need to show.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I didn't test what happens if I make a preload a netvm of a qube, need to test...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, there is no such situation, but if the user chooses to make it, we need to show.

but here and everywhere else this function is used you are deliberately excluding preloaded dispvms, so I think they should just be excluded by the function itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I didn't test what happens if I make a preload a netvm of a qube, need to test...

I meant the opposite, cause being an audiovm or netvm is handled by QubesOS/qubes-core-admin#733. Preloaded disposables can theoretically be one other property, guivm of a qube... not too useful right now but there is nothing blocking it in qubes.ext.gui.

but here and everywhere else this function is used you are deliberately excluding preloaded dispvms, so I think they should just be excluded by the function itself?

You are right. I think the tests can stay, here is the only place where a qube can be renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or not even tests, because it becomes irrelevant one qubesadmin is fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so this issue remains - I think the function vm_dependencies should do the filtering, so that here you wouldn't need a list comprehension (and just use it like in previous versions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am closing this PR in favor of:

]

if dependencies:
list_deps = manager_utils.format_dependencies_list(dependencies)
Expand Down
17 changes: 13 additions & 4 deletions qubesmanager/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 not vm or (vm and not utils.is_preload(vm))
]

if dependencies:
list_text = utils.format_dependencies_list(dependencies)
Expand Down Expand Up @@ -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:
Expand Down
14 changes: 8 additions & 6 deletions qubesmanager/tests/test_qube_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
Expand Down
123 changes: 105 additions & 18 deletions qubesmanager/tests/test_vm_settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand All @@ -38,6 +40,10 @@
"test-standalone",
"test-old",
"test-vm-set",
"default-dvm",
"test-disp",
"test-alt-dvm",
"test-alt-disp",
]

# with a template
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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 identical names on both mock_app may and the current system
# may affects test, names such as sys-net.
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
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this comment means. Same names to what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of still don't get it; the system has just what is in the mock, it's quite expected and not "may be there or not"?

# 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")
Expand Down
11 changes: 10 additions & 1 deletion qubesmanager/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down