labgrid/qemudriver: tie QEMU and QMP monitor start/stop to on_activate()/on_deactivate() #1771
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since `qemu_version_mock` is required for all tests using `qemu_mock`, move its functionality into `qemu_mock`. Since `qemu_driver` will fixture will soon need `qemu_mock` always, add it as its dependency now, thus minimizing the direct fixtures required by the tests. Signed-off-by: Bastian Krause <bst@pengutronix.de>
…e()/on_deactivate() Until now the QEMU process and QMP monitor start was tied to the on()/off() methods. This feels unnatural, preventing the user from interacting with the QEMU process via monitor commands before the emulation starts and meant starting a new process on each power cycle. Rework the driver to start QEMU and the QMP monitor in on_activate(), allowing interaction via monitor_command after activation. The on() and off() methods interact only via QMP now. All methods relying on a started QEMU and QMP monitor instance are decorated with @Driver.check_active now. The atexit handling is no longer required since the target's atexit handler already calls the driver's on_deactivate(). Signed-off-by: Joschka Seydell <joschka@seydell.org> Signed-off-by: Bastian Krause <bst@pengutronix.de>
Now that the QEMU invocation is not only prepared, but also started in on_activate(), there is no need to keep the command around. Turn it into a local variable instead. Signed-off-by: Bastian Krause <bst@pengutronix.de>
3 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1771 +/- ##
========================================
- Coverage 45.3% 45.2% -0.1%
========================================
Files 172 172
Lines 13517 13556 +39
========================================
+ Hits 6130 6140 +10
- Misses 7387 7416 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Thanks for addressing this, @Bastian-Krause ! Seems indeed more adherent to the common design/usage patterns. I'll give it a closer look as soon as time permits. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Until now the QEMU process and QMP monitor start was tied to the
on()/off()methods. This feels unnatural, preventing the user from interacting with the QEMU process via monitor commands before the emulation starts and meant starting a new process on each power cycle.Rework the driver to start QEMU and the QMP monitor in on_activate(), allowing interaction via
monitor_command()after activation. Theon()andoff()methods interact only via QMP now.All methods relying on a started QEMU and QMP monitor instance are decorated with
@Driver.check_activenow.The atexit handling is no longer required since the target's atexit handler already calls the driver's
on_deactivate().While at it, restructure the QEMU mock fixtures in the QEMUDriver tests.
Checklist
Alternative to one aspect of #1753