Conversation
052b35b to
2a42699
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #438 +/- ##
==========================================
+ Coverage 76.30% 76.36% +0.06%
==========================================
Files 53 53
Lines 9363 9388 +25
==========================================
+ Hits 7144 7169 +25
Misses 2219 2219 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
qubesadmin/base.py
Outdated
| # TODO what if `dest` remains None here ? | ||
| # qubesd_call expects a non-None `dest` arg | ||
| dest: QubesVM | None = getattr(self.app, "default_dispvm", None) | ||
| assert dest is not None |
There was a problem hiding this comment.
It can be None, please remove this assertion. Let qubesd reply with the exception so the caller knows what happened.
There was a problem hiding this comment.
I think this comment belongs to #436 but we can have the conversation here.
Just to be clear: I also had to add an assert is not None in qubesd_call when payload_stream is not None, otherwise we raise a TypeError: unsupported operand type(s) for +: 'NoneType' ...'
Can you confirm that what you're suggesting is to remove the above-mentioned first assert dest is no None (L97), but not the second one (L884) ?
Note: even if payload_stream is None, dest is then used in call_header = "{}+{} dom0 name {}\0".format(method, arg or "", dest), and I would be quite surprised if a cast to "None" rather than to "" is the expected behavior here, can you confirm ?
There was a problem hiding this comment.
Added some logging:
In qubesadmin module:
diff --git a/qubesadmin/base.py b/qubesadmin/base.py
index b07af14..91253a1 100644
--- a/qubesadmin/base.py
+++ b/qubesadmin/base.py
@@ -84,6 +84,7 @@ class PropertyHolder:
if dest:
dest = dest.name
# have the actual implementation at Qubes() instance
+ self.app.log.warning("TTT: dest=%s method=%s arg=%s", dest, method, arg)
return self.app.qubesd_call(dest, method, arg, payload,
payload_stream)
In qubes module:
diff --git a/qubes/api/admin.py b/qubes/api/admin.py
index 2b86b4ce..599a636a 100644
--- a/qubes/api/admin.py
+++ b/qubes/api/admin.py
@@ -1337,6 +1337,7 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
appvm = self.src.default_dispvm
else:
appvm = self.dest
+ self.app.log.warning("TTT: src=%s dest=%s appvm=%s", self.src, self.dest, appvm)
self.fire_event_for_permission(dispvm_template=appvm)
if preload:
diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py
index 2f8edcd4..06ec54f8 100644
--- a/qubes/vm/dispvm.py
+++ b/qubes/vm/dispvm.py
@@ -786,7 +786,7 @@ class DispVM(qubes.vm.qubesvm.QubesVM):
if not getattr(appvm, "template_for_dispvms", False):
raise qubes.exc.QubesException(
"Refusing to create disposable out of app qube which has "
- "template_for_dispvms=False"
+ "template_for_dispvms=False out of %s" % appvm
)
if preload and not appvm.can_preload():
# Using an exception clutters the log when 'used' event isUnset the dom0 disposable template:
qvm-prefs dom0 default_dispvm ''
Unset the global disposable template (just to avoid confusion):
qubes-prefs default_dispvm ''
And then:
% qvm-run -p --dispvm -- echo hey
app: TTT: dest=None method=admin.vm.feature.CheckWithTemplate arg=vmexec
@dispvm: Refusing to create disposable out of app qube which has template_for_dispvms=False out of None
qubesd[261121]: WARNING: TTT: src=dom0 dest=dom0 appvm=None
So, the client passing None, the server translates to dom0 somewhere. So I am unsure if you should block it or not. I guess OpenQA will tell.
However, in this particular case, I think I made a mistake, it shouldn't try to get self.app.default_dispvm, but instead, should be "dom0", as a string, not a qube object.
We can't pass dest="", it may fail depending if we are using VMExec or VMShell, the interaction is a bit weird because of the different calls it needs to make in qubesadmin.tools.qvm_run:
% qvm-run -p --dispvm -- echo hey
app: TTT: dest= method=admin.vm.feature.CheckWithTemplate arg=vmexec
@dispvm: VM name contains illegal characters
zsh: exit 255 qvm-run -p --dispvm -- echo hey
% qvm-run -p --dispvm -- 'echo hey'
app: TTT: dest=disp5555 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: dest=disp5555 method=admin.vm.feature.CheckWithTemplate arg=os
hey
app: TTT: dest=disp5555 method=admin.vm.Kill arg=None
We can pass dest=None:
% qvm-run -p --dispvm -- echo hey
app: TTT: dest=None method=admin.vm.feature.CheckWithTemplate arg=vmexec
app: TTT: dest=disp6178 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: dest=disp6178 method=admin.vm.feature.CheckWithTemplate arg=os
hey
app: TTT: dest=disp6178 method=admin.vm.Kill arg=None
% qvm-run -p --dispvm -- 'echo hey'
app: TTT: dest=disp4672 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: dest=disp4672 method=admin.vm.feature.CheckWithTemplate arg=os
hey
app: TTT: dest=disp4672 method=admin.vm.Kill arg=None
And we can pass dest=dom0:
% qvm-run -p --dispvm -- echo hey
app: TTT: dest=dom0 method=admin.vm.feature.CheckWithTemplate arg=vmexec
app: TTT: dest=disp8458 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: dest=disp8458 method=admin.vm.feature.CheckWithTemplate arg=os
hey
app: TTT: dest=disp8458 method=admin.vm.Kill arg=None
% qvm-run -p --dispvm -- 'echo hey'
app: TTT: dest=disp4995 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: dest=disp4995 method=admin.vm.feature.CheckWithTemplate arg=os
hey
app: TTT: dest=disp4995 method=admin.vm.Kill arg=None
However, in this particular case, I think I made a mistake, it shouldn't try to get
self.app.default_dispvm, but instead, should be"dom0", as a string, not a qube object.
I will explain this a bit more in depth. The client doesn't need to know the system/global default disposable template, they just need to destine their call to dom0. It is better to use dest="dom0" then, because of how it is handled on the server:
Using the caller (src) default disposable template.
Not to the point of call_header, it seems that it is currently passed as None and works, if this is correct and if the server does some transformation, I don't know, but it works:
% qvm-run -p --dispvm -- echo hey
app: TTT: dest=None method=admin.vm.feature.CheckWithTemplate arg=vmexec
app: TTT: QubesLocal.qubesd_call header: admin.vm.feature.CheckWithTemplate+vmexec dom0 name None
app: TTT: QubesLocal.qubesd_call header: admin.vm.CreateDisposable+ dom0 name dom0
app: TTT: dest=disp220 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: QubesLocal.qubesd_call header: admin.vm.property.Get+qrexec_timeout dom0 name disp220
app: TTT: QubesLocal.qubesd_call header: admin.vm.Start+ dom0 name disp220
app: TTT: dest=disp220 method=admin.vm.feature.CheckWithTemplate arg=os
app: TTT: QubesLocal.qubesd_call header: admin.vm.feature.CheckWithTemplate+os dom0 name disp220
hey
app: TTT: dest=disp220 method=admin.vm.Kill arg=None
app: TTT: QubesLocal.qubesd_call header: admin.vm.Kill+ dom0 name disp220
% qvm-run -p --dispvm -- 'echo hey'
app: TTT: QubesLocal.qubesd_call header: admin.vm.CreateDisposable+ dom0 name dom0
app: TTT: dest=disp7281 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: QubesLocal.qubesd_call header: admin.vm.property.Get+qrexec_timeout dom0 name disp7281
app: TTT: QubesLocal.qubesd_call header: admin.vm.Start+ dom0 name disp7281
app: TTT: dest=disp7281 method=admin.vm.feature.CheckWithTemplate arg=os
app: TTT: QubesLocal.qubesd_call header: admin.vm.feature.CheckWithTemplate+os dom0 name disp7281
hey
app: TTT: dest=disp7281 method=admin.vm.Kill arg=None
app: TTT: QubesLocal.qubesd_call header: admin.vm.Kill+ dom0 name disp7281
If I use on the call_header, dest or "", it doesn't work always, e.g. with VMExec it fails, with VMShell, it never reached that point, so it was not impacted, but if it reached, it would most likely break also:
% qvm-run -p --dispvm -- echo hey
app: TTT: dest=None method=admin.vm.feature.CheckWithTemplate arg=vmexec
app: TTT: QubesLocal.qubesd_call header: admin.vm.feature.CheckWithTemplate+vmexec dom0 name
@dispvm: VM name contains illegal characters
zsh: exit 255 qvm-run -p --dispvm -- echo hey
% qvm-run -p --dispvm -- 'echo hey'
app: TTT: QubesLocal.qubesd_call header: admin.vm.CreateDisposable+ dom0 name dom0
app: TTT: dest=disp4346 method=admin.vm.property.Get arg=qrexec_timeout
app: TTT: QubesLocal.qubesd_call header: admin.vm.property.Get+qrexec_timeout dom0 name disp4346
app: TTT: QubesLocal.qubesd_call header: admin.vm.Start+ dom0 name disp4346
app: TTT: dest=disp4346 method=admin.vm.feature.CheckWithTemplate arg=os
app: TTT: QubesLocal.qubesd_call header: admin.vm.feature.CheckWithTemplate+os dom0 name disp4346
hey
app: TTT: dest=disp4346 method=admin.vm.Kill arg=None
app: TTT: QubesLocal.qubesd_call header: admin.vm.Kill+ dom0 name disp4346
There was a problem hiding this comment.
tl;dr: app.qubesd_call() should never get dest=None (it's an error, should raise an exception). It's okay to have dest=None at vm.qubesd_call(), which will translate it to vm._method_dest.
The thing that happens above is bizarre, and "thanks" to lack of enforcement of dest!=None, it ends up producing weird results. Specifically:
- Note this is about
admin.vm.feature.CheckWithTemplatemethod, so it got called fromQubesVM.check_with_template(). It will be important later. QubesLocal.qubesd_call()will actually send call withstr(None)as destination.- Qubesd will get it, and will (unsuccessfully) try to find such VM.
- Qubesd will respond with
QubesVMNotFoundError("None"). - Now, back in
QubesVM.check_with_template()you getQubesVMNotFoundError("None"), which happen to inherit fromKeyError, so gets handled as "no such feature" and default value is used.
There was a problem hiding this comment.
In this particular case, the issue is in PropertyHolder.qubesd_call() - it should verify if default_dispvm it got is not None, and refuse to continue otherwise (not sure what exception would be most appropriate, but the user should get a message about default dispvm not set).
There was a problem hiding this comment.
It is hardcoding dom0 because of this: https://github.com/QubesOS/qubes-core-admin/blob/53ca30fc3257ac8174ae6a6d37dce70f72e788d5/qubes/api/admin.py#L1336
Calling @dispvm with dest=dom0 gets the dom0.default_dispvm, which defaults to app.default_dispvm.
I will use , I will use local_namedom0, as local is the current qube that loaded the qubesadmin, which might not be dom0.
But then, the message should be clearer why this happened, maybe "Requested default disposable, but 'default_dispvm' property is empty"?
As for the exception, maybe justQubesVMNotFoundError? Or if going with a new one, sometime likeQubesDefaultNotSetError?
I don't like QubesDefaultNotSetError the second because the dom0.default_dispvm can use a literal qube reference instead of a default value to use app.deault_dispvm.
I think I will settle for QubesVMNotFoundError, I just wanted to do a better exception.
There was a problem hiding this comment.
Calling
@dispvmwithdest=dom0gets thedom0.default_dispvm, which defaults toapp.default_dispvm.
Correction, it gets src.default_dispvm, which depends on the calling qube property..., so yes, indeed local_name fits better.
There was a problem hiding this comment.
This PR to qubes-builderv2 for reference:
There was a problem hiding this comment.
Some weird recursion error when using self.app.domains[self.local_name] inside qubesd_call...
There was a problem hiding this comment.
that should be self.app.local_name...
Replied in the review above.
Same response as above.
Yes, I don't think
I don't think it should fail, but instead omit |
d1485e6 to
133e678
Compare
Not sure I understand if you mean that it is not allowed to ever be None (in which case we should raise an explicit error, or simply remove the
|
133e678 to
85d178b
Compare
It seems that |
Technically, the list can be dynamic. Add-on to core-admin can introduce new device classes - for example once you install
|
OK
Imo should be "fixed" by #445 so not blocking anymore for this PR |
85d178b to
3ae2de5
Compare
|
Regarding the discussion around |
|
It can be in separate PR (adjusting type hint, adding assert, and handling the case of default_dispvm=None). |
a1e67cd to
5c4e103
Compare
|
I believe I resolved the comments above |
5c4e103 to
ad6d3d6
Compare
ad6d3d6 to
6f79989
Compare
marmarek
left a comment
There was a problem hiding this comment.
Please squash "Cleanup dynamic types" commit into the typing one, as it simply undoes some changes.
6f79989 to
8fa5e85
Compare
* origin/pr/440: type-check devices.py exc.py features.py Pull request description: Follows #438
Follows #437
Comments / Questions
_vm_list
_vm_listis no a list - it's a dict. I renamed it accordingly._vm_dict_initializedvariable that explicitly tracks the initialisation rather than relying onis None. (which avoids puttingassert is not Noneeverywhere)_vm_listand_vm_objects. Would be happy to add it to the PR if I get some suggestions.deviceclass
I would like to make
deviceclassaLiterallist of allowed strings, but I'm not sure what the exhaustive list of allowed classes are ?Safe asserts
is not Nonevolume.nameis notNone?dst_volume.namea bit after:qubesd_callwe havedest=Nonebut this is not mentioned in the docstringargisNonesince _call_with_stream excepts alist[str], notlist[str|None]. Is that OK ?