Skip to content

Conversation

@ben-grande
Copy link
Contributor

@ben-grande ben-grande commented Dec 3, 2025

Yes, probable, we can't know for sure until the policy evaluates the
call to @disvpm, which is unfortunately, too late for some things, such
as querying properties and features to know how to best approach the
call, with GUI or without, with VMExec or VMShell.

The check is only done if the disposable class is instantiated asking
for such override. By fixing this issue, it is possible to have multiple
arguments in qvm-run that target disposables, which will use VMShell.

Another bug fixed is an exception raised when attempting to get the GUI
settings of the disposable, in case the disposable template name was
typed erroneously, introduced in
cc9c5f5.

$ time qvm-run -p -v --dispvm=unexistent -- echo hi
Traceback (most recent call last):
File "/usr/bin/qvm-run", line 5, in
sys.exit(main())
~~~~^^
File "/usr/lib/python3.13/site-packages/qubesadmin/tools/qvm_run.py", line 367, in main
else args.app.domains[args.dispvm]
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
File "/usr/lib/python3.13/site-packages/qubesadmin/app.py", line 108, in getitem
raise KeyError(item)
KeyError: 'unexistent'

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

@marmarek
Copy link
Member

marmarek commented Dec 3, 2025

Generally, I don't want more uses of qubes.VMShell instead of qubes.VMExec. The former has a ton of pitfalls related to quoting, newlines, policy, logging etc. qubes.VMShell is used if the command is a single string (which possibly is a shell command) for compatibility but otherwise qubes.VMExec should be preferred.

@ben-grande
Copy link
Contributor Author

ben-grande commented Dec 24, 2025

The issue is qubesadmin.features.Features.check_with_template, it uses self.vm.name that resolves to @dispvm or @dispvm:default-dvm.

This is what qubesadmin/features nows of vars(self.vm):

CHECKING FEATURE vmexec: {'app': <qubesadmin.app.QubesLocal object at 0x711351e22e40>, '_method_prefix': 'admin.vm.property.', '_method_dest': '$dispvm', '_properties': None, '_properties_help': None, '_properties_cache': {}, '_volumes': None, '_klass': None, '_power_state_cache': None, 'log': <Logger $dispvm (INFO)>, 'tags': <qubesadmin.tags.Tags object at 0x711351e234d0>, 'features': <qubesadmin.features.Features object at 0x711351e23620>, 'devices': {}, 'firewall': <qubesadmin.firewall.Firewall object at 0x711351e23770>}

It doesn't yet know the template of $dispvm because it depends on the global property. One solution might be to create the disposable earlier than attempting the call.

@ben-grande ben-grande marked this pull request as ready for review December 24, 2025 10:53
@ben-grande
Copy link
Contributor Author

There is another issue:

% time qvm-run -p -v --dispvm=default-dvma -- echo hi
Traceback (most recent call last):
  File "/usr/bin/qvm-run", line 5, in <module>
    sys.exit(main())
             ~~~~^^
  File "/usr/lib/python3.13/site-packages/qubesadmin/tools/qvm_run.py", line 367, in main
    else args.app.domains[args.dispvm]
         ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
  File "/usr/lib/python3.13/site-packages/qubesadmin/app.py", line 108, in __getitem__
    raise KeyError(item)
KeyError: 'default-dvma'
zsh: exit 1     qvm-run -p -v --dispvm=default-dvma -- echo hi

Because the check is too early, I will move to run_command_single() for disposables.

@codecov
Copy link

codecov bot commented Dec 24, 2025

Codecov Report

❌ Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.22%. Comparing base (dcb1ec8) to head (60f91b1).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
qubesadmin/base.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
+ Coverage   76.16%   76.22%   +0.06%     
==========================================
  Files          53       53              
  Lines        9304     9352      +48     
==========================================
+ Hits         7086     7129      +43     
- Misses       2218     2223       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ben-grande
Copy link
Contributor Author

Because the check is too early, I will move to run_command_single() for disposables.

This means that the disposable has to be created when using any method, vmexec or not, so it is possible to check has_gui().

@marmarek
Copy link
Member

% time qvm-run -p -v --dispvm=default-dvma -- echo hi

This one looks like a typo: default-dvma != default-dvm. Checking relevant features on default-dvm (or other selected disposable template) should be enough.

@ben-grande
Copy link
Contributor Author

ben-grande commented Dec 25, 2025 via email

@marmarek
Copy link
Member

But with the latest commit you again beaks usage of standard @dispvm:... policy lines...

@ben-grande
Copy link
Contributor Author

But with the latest commit you again beaks usage of standard @dispvm:... policy lines...

Ugh, right.

Another try, overriding the destination on base.py qubesd_call():

diff --git a/qubesadmin/base.py b/qubesadmin/base.py
index 05e3027..3dd6687 100644
--- a/qubesadmin/base.py
+++ b/qubesadmin/base.py
@@ -73,6 +73,11 @@ class PropertyHolder(object):
             dest = self._method_dest
         # have the actual implementation at Qubes() instance
         if self.app:
+            if dest.startswith("@dispvm"):
+                if dest.startswith("@dispvm:"):
+                    dest = dest[len("@dispvm:") :]
+                else:
+                    dest = getattr(self.app, "default_dispvm", None)
             return self.app.qubesd_call(dest, method, arg, payload,
                 payload_stream)
         raise NotImplementedError
diff --git a/qubesadmin/tools/qvm_run.py b/qubesadmin/tools/qvm_run.py
index 064bc75..449493f 100644
--- a/qubesadmin/tools/qvm_run.py
+++ b/qubesadmin/tools/qvm_run.py
@@ -257,8 +257,6 @@ def run_command_single(args, vm):
         args.cmd_args.insert(0, args.cmd)
         args.cmd = args.VMNAME
         args.VMNAME = None
-        if args.dispvm:
-            vm.create_disposable()

     use_exec = len(args.cmd_args) > 0 or args.no_shell

Running the tests shows that two admin calls that will need to be allowed:

call ('test-vm', 'admin.vm.feature.CheckWithTemplate', 'vmexec', None)
call ('dom0', 'admin.property.Get', 'default_dispvm', None)

For the second call, I tested without a global default_dispvm and the error message is okayish on None: Refusing to create disposable out of this AppVM, because template_for_dispvms=False

The first call is getting the vmexec feature from the disposable template. It is not nice for preloaded disposables as it is not truthful to the disposable properties and features.

Is this ok?

@ben-grande
Copy link
Contributor Author

The first call is getting the vmexec feature from the disposable template. It is not nice for preloaded disposables as it is not truthful to the disposable properties and features.

Is this ok?

Not ok, for the dom0 call also.

#401 (comment)

This is a bad idea. Normal qubes intentionally have no permission to call admin.vm.CreateDisposable (nor any other admin.* calls), but they may be granted calls to the @dispvm target (possibly choosing different disposable template depending on the requested service).

Requiring extended admin API access just to measure time a bit better is no-go...

It may not reflect the truth when redirecting target per service via policy...

Out of ideas if the admin.vm.CreateDisposable cannot be made early as it would circumvent policy rules for @dispvm. From what I know, only Qubes Builder is using that call for finer control of the disposable. Either GUI domain has that capability or I guess, multiple qvm-run args with --dispvm* remains unsupported and I will add an error message for that case, as I have no idea anymore.

@marmarek
Copy link
Member

Note qvm-run != qvm-run-vm. The latter works with minimal set of permissions, the former requires at least ability to list VMs. It might be okay to check for the vmexec feature there too, but with a PermissionDenied exception handling (and logging warning in that case?).

The first call is getting the vmexec feature from the disposable template. It is not nice for preloaded disposables as it is not truthful to the disposable properties and features.

Well, preloaded disposables are not supposed to be modified, so IMO it's okay to assume they have the same configuration as freshly created disposable.

@ben-grande
Copy link
Contributor Author

Well, preloaded disposables are not supposed to be modified, so IMO it's okay to assume they have the same configuration as freshly created disposable.

Now it is truthful for properties:

I don't think features are relevant because:

  • most checks use check_with_template, so changing a feature on the disposable template is fine
  • changing a feature on the disposable has to be something the user manually does, but that is hidden from GUI applications

@ben-grande
Copy link
Contributor Author

Well, preloaded disposables are not supposed to be modified, so IMO it's okay to assume they have the same configuration as freshly created disposable.

Yes, but also the feature might be checking the wrong template as the policy is not checked if it would redirect the target.

From our discussion, the decision was to make something for qvm_run.py only or maybe something that can be reused by other tools in the feature, that would use --dispvm, such as qvm-device. Although imperfect solution, qvm-run just checks a few things (for now) before creating the disposable:

  • Feature(s): vmexec, gui
  • Property(ies): guivm

Lacking vmexec is not the case for some years, but differing values in gui and guivm could be a concern, that would require the user specifying --(no-)?gui.

This is why it cannot be the default in qubesadmin.base.PropertyHolder.qubesd_call, it needs to be checked carefully on each added script.i

Yes, probable, we can't know for sure until the policy evaluates the
call to @disvpm, which is unfortunately, too late for some things, such
as querying properties and features to know how to best approach the
call, with GUI or without, with VMExec or VMShell.

The check is only done if the disposable class is instantiated asking
for such override. By fixing this issue, it is possible to have multiple
arguments in qvm-run that target disposables, which will use VMShell.

Another bug fixed is an exception raised when attempting to get the GUI
settings of the disposable, in case the disposable template name was
typed erroneously, introduced in
cc9c5f5.

$ time qvm-run -p -v --dispvm=unexistent -- echo hi
Traceback (most recent call last):
  File "/usr/bin/qvm-run", line 5, in <module>
    sys.exit(main())
             ~~~~^^
  File "/usr/lib/python3.13/site-packages/qubesadmin/tools/qvm_run.py", line 367, in main
    else args.app.domains[args.dispvm]
         ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
  File "/usr/lib/python3.13/site-packages/qubesadmin/app.py", line 108, in __getitem__
    raise KeyError(item)
KeyError: 'unexistent'

Fixes: QubesOS/qubes-issues#10383
For: QubesOS/qubes-issues#1512
@ben-grande ben-grande changed the title Allow multiple qvm-run args on disposable calls Check dummy disp properties vs probable template Jan 9, 2026
@marmarek
Copy link
Member

PipelineRetry

@marmarek marmarek merged commit 552f17a into QubesOS:main Jan 13, 2026
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

qvm-run --dispvm doesn't work with multiple arguments

2 participants