Add boot mode support

[ArrayBolt3]

This PR is intended to add support for boot modes as specified here: QubesOS/qubes-issues#9750 (comment) This naturally only handles the backend part of things, the frontend will need its own PR against qubes-manager.

This is incomplete - I have not yet actually added the part that listens for qvm-request-features calls from TemplateVMs and sets boot mode features appropriately.

This implements (or rather, will implement) the backend component needed to implement QubesOS/qubes-issues#9750.

[ArrayBolt3]

Most of the backend is now working, however see QubesOS/qubes-issues#9775, which blocks this from being fully functional. (EDIT: The linked bug report has been fixed by Marek, thus this is no longer blocked.)

[ArrayBolt3]

This is now ready for review. This should be merged at the same time as the corresponding frontend PR, QubesOS/qubes-manager#407.

[marmarek]

The first thing I see - this needs unit tests. See qubes/tests/ dir. You are most likely interested in ext.py and vm/qubes.py there.

[ArrayBolt3]

Yep, I figured those were probably necessary. I'm working on those now.

[ArrayBolt3]

Unit tests now added (and broken ones fixed). They may need to be enhanced though, not all of the new code is tested because I haven't figured out how to make the defaults mechanism work seamlessly in the mocks. Not sure if that's even possible, or desirable.

[marmarek]

and see also CI - both pylint and black complain

[ArrayBolt3]

Thanks for the very thorough review! Will work through these issues and then request another review once ready.

[ArrayBolt3]

@marmarek What would you think about me reorganizing the boot mode feature names from boot-mode.bootmodename.kernelopts to boot-mode.kernelopts.bootmodename and the like? Right now I'm using regex matching to find boot mode related features, when I could be using str.startswith() if it was flipped around. That would probably come with performance advantages and less risk of security-related issues.

[marmarek]

@marmarek What would you think about me reorganizing the boot mode feature names from boot-mode.bootmodename.kernelopts to boot-mode.kernelopts.bootmodename and the like? Right now I'm using regex matching to find boot mode related features, when I could be using str.startswith() if it was flipped around. That would probably come with performance advantages and less risk of security-related issues.

Fine with me.

[ArrayBolt3]

Still need to deal with pylint and black issues, the ext unit tests, and look at the default boot mode name stuff a bit more. Pushed what I had so far since it resolved most issues.

[ArrayBolt3]

This should now be ready for another review. I still haven't figured out the default boot mode name stuff, but everything else should now be dealt with.

[marmarek]

black issues

hint: black -l80 qubes will do the job for you

[ArrayBolt3]

@marmarek The problem I'm running into with trying to add a default boot mode name is that in qubes-builderv2, initialize_widget_for_property, the value used to fill in the default entry is the value of the property, not the associated name of the property. I think that could probably be fixed, and if so, adding a default boot mode name should be doable.

[ArrayBolt3]

Did another push because black had to reformat some things.

[marmarek]

pylint complains...

[ArrayBolt3]

Fighting with event handler regression test issues, not ready for review yet...

[ArrayBolt3]

@marmarek I tried to fix the pylint issues, which seems to have made it happy, but the regression tests now fail:

======================================================================
ERROR: qubes.tests.vm.qubesvm/TC_90_QubesVM/test_811_default_bootmode
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.13/contextlib.py", line 85, in inner
    return func(*args, **kwds)
  File "/home/user/qubes-core-admin/qubes/tests/vm/qubesvm.py", line 3121, in test_811_default_bootmode
    vm.features["boot-mode.active"] = "testmode1"
    ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^
  File "/home/user/qubes-core-admin/qubes/features.py", line 93, in __setitem__
    self.subject.fire_event(
    ~~~~~~~~~~~~~~~~~~~~~~~^
        "domain-feature-set:" + key, feature=key, value=value
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/user/qubes-core-admin/qubes/events.py", line 200, in fire_event
    sync_effects, async_effects = self._fire_event(
                                  ~~~~~~~~~~~~~~~~^
        event, kwargs, pre_event=pre_event
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/home/user/qubes-core-admin/qubes/events.py", line 169, in _fire_event
    effect = func(self, event, **kwargs)
TypeError: QubesVM.on_feature_bootmode_active_set() missing 1 required positional argument: 'event'

I can't figure out why this happens though - qubes/vm/qubesvm.py -> on_feature_bootmode_active_set uses this "signature" (not sure what these are called in Python):

    @qubes.events.handler("domain-feature-set:boot-mode.active")
    def on_feature_bootmode_active_set(
        self, vm, event, feature, value, oldvalue=None
    ):

This is nearly identical to a signature in qubes/ext/services.py -> on_domain_feature_set:

    @qubes.ext.handler("domain-feature-set:*")
    def on_domain_feature_set(self, vm, event, feature, value, oldvalue=None):

How come the second one works but the first one doesn't? (The first one used to work without problems before adding self as the first argument, as required by pylint.)

[marmarek]

self, vm,

self is vm, don't need to repeat it. It's the difference between event handler defined in the VM class itself (@qubes.events.handler) and in an extension (@qubes.ext.handler)

[ArrayBolt3]

Oh sheesh. I stared at this code for like fifteen minutes and didn't notice the qubes.ext.handler vs qubes.events.handler bit. Guess that'll teach me to use a visual diff tool rather than comparing side-by-side in Vim 😅

[ArrayBolt3]

Fixed the issue with the failing regression tests.

[codecov]

Codecov Report

Attention: Patch coverage is 99.01961% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.07%. Comparing base (5df3ba5) to head (e9c62be).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
qubes/vm/qubesvm.py	97.43%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #653      +/-   ##
==========================================
+ Coverage   69.90%   70.07%   +0.16%     
==========================================
  Files          59       59              
  Lines       12612    12683      +71     
==========================================
+ Hits         8817     8888      +71     
  Misses       3795     3795              

Flag	Coverage Δ
unittests	70.07% <99.01%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

[ArrayBolt3]

@marmarek How would I run the code coverage test locally? It looks like there's some missed stuff I would have liked to have caught on my end earlier. Currently working on resolving that now.

[marmarek]

The run-tests scripts does run with code coverage enabled. You can get the report with coverage tool (or python3 -m coverage)

[ArrayBolt3]

qubesvm.py line 1010 should be unreachable code, thus why it shows as not covered. (_default_bootmode() shouldn't ever return something that isn't found by self.features.check_with_template(), except of course for the default value.) All the other code coverage issues should now be solved.

[marmarek]

Black complains about one tiny thing...

[ArrayBolt3]

Should be fixed, I'll try to get in the habit of running it before all pushes.

(aside, qubes-manager is NOT formatted by black, and I've caused myself major headaches more than once thinking I'm supposed to reformat it after changing it and then realizing I just changed like 90% of the code in a way that's tricky to revert if you had uncommitted, unstaged changes, or were silly enough to run git add -A after that. Is there a reason it isn't "blackened"?)

[marmarek]

Is there a reason it isn't "blackened"

It isn't yet