labgrid icon indicating copy to clipboard operation
labgrid copied to clipboard

qemudriver: Allow machine-specific options and pre-start QMP commands

Open JSydll opened this issue 3 months ago • 10 comments

Description

Two minor features are added to the QemuDriver:

Support machine-specific options (e.g. -machine virt,secure=on)

While raising an exception for unsupported machines/boards is reasonable, adding machine-specific options shall not break the parsing. Therefore, the check is adjusted to only account for the leading board name.

Support interactions with the QEMU instance via QMP before releasing CPU(s)

Especially for bad case testing, it can be useful to be able to interact with the prepared but not yet spun off instance - e.g. to corrupt the boot medium without the need to build a corrupted image only for that purpose.

For both features, straight forward unit tests are provided. Lastly, the changeset includes minor documentation updates and the introduction of type hints for the qemudriver.py file.

Checklist

  • [x] Documentation for the feature
  • [x] Tests for the feature
  • [x] PR has been tested

JSydll avatar Oct 06 '25 04:10 JSydll

Thanks for the review and sorry for force-pushing so often - it looked easier to me to fix the one linting issue I overlooked when implementing the proposal from Rouven.

JSydll avatar Nov 03 '25 19:11 JSydll

It's all good and thanks for keeping up with the PR even if it takes some time on our side :+1:

Emantor avatar Nov 04 '25 08:11 Emantor

I shortly glimpsed into the failing pipeline and now I'm wondering whether some environment changes happened? The test_qemudriver is not able to locate qemu-system-arm...

JSydll avatar Nov 04 '25 09:11 JSydll

Codecov Report

:x: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 45.4%. Comparing base (f63dfd7) to head (cdc68c3). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
labgrid/driver/qemudriver.py 93.9% 2 Missing :warning:
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1753     +/-   ##
========================================
+ Coverage    45.3%   45.4%   +0.1%     
========================================
  Files         172     172             
  Lines       13517   13523      +6     
========================================
+ Hits         6130    6151     +21     
+ Misses       7387    7372     -15     
Flag Coverage Δ
3.10 45.4% <93.9%> (+0.1%) :arrow_up:
3.11 45.4% <93.9%> (+0.1%) :arrow_up:
3.12 45.4% <93.9%> (+0.1%) :arrow_up:
3.13 45.4% <93.9%> (+0.1%) :arrow_up:
3.14 45.4% <93.9%> (+0.1%) :arrow_up:
3.9 45.5% <93.9%> (+0.1%) :arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 04 '25 21:11 codecov[bot]

Prior to this, we've mocked qemu. Why do we need to use the real binary now?

If we decide we need the real qemu, we should at least add a marker so the tests get skipped on systems without it. Something along the lines of:

https://github.com/labgrid-project/labgrid/blob/f63dfd70a5da2c7007ee8c320fa969df2c0bf16e/tests/test_sigrok.py#L11-L12

Bastian-Krause avatar Nov 05 '25 07:11 Bastian-Krause

Hmm, I'm actually not sure where this dependency is now coming from. It's not like the tests I want add depend on it. Maybe I forgot to inject the qemu_mock in one of the test cases? Could that be the root cause? I'll try this as soon as time permits.

JSydll avatar Nov 12 '25 12:11 JSydll

I think I found the culprit - a missing qemu_version_mock - I overlooked the dependency to this in the get_qemu_base_args. Unfortunately, I wasn't able to test the workflow locally - could you retrigger the pipeline, @Bastian-Krause or @Emantor ? I reverted the change to the docker container so it should be quickly visible whether I now identified all missing parts...

JSydll avatar Nov 13 '25 08:11 JSydll

After looking at the QEMUDriver again, I'm not too happy about starting the qemu process in on() (or with this PR in prepare() and stopping it in off(). This is not the fault of this PR, though. This PR only makes it even more visible.

I'll create an alternative PR starting qemu during on_activate() and stopping it in on_deactivate(). on() and off() will interact with it via QMP only. This should cover your use case because you can call monitor_command() right after activation. If we agree that this is a good solution, you can rebase your other changes on top.

Bastian-Krause avatar Nov 13 '25 14:11 Bastian-Krause

After looking at the QEMUDriver again, I'm not too happy about starting the qemu process in on() (or with this PR in prepare() and stopping it in off(). This is not the fault of this PR, though. This PR only makes it even more visible.

I'll create an alternative PR starting qemu during on_activate() and stopping it in on_deactivate(). on() and off() will interact with it via QMP only. This should cover your use case because you can call monitor_command() right after activation. If we agree that this is a good solution, you can rebase your other changes on top.

See #1771.

Bastian-Krause avatar Nov 14 '25 17:11 Bastian-Krause

Makes sense. I haven't expected a proposal so early, thanks!

JSydll avatar Nov 14 '25 20:11 JSydll