Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qemudriver: export get_qemu_base_args method #1212

Merged
merged 3 commits into from Sep 6, 2023

Conversation

a3f
Copy link
Contributor

@a3f a3f commented Jun 16, 2023

For debugging, it can be useful to start an interactive session for the
user to access the DUT console. This is possible with real targets
through labgrid-client -s STATE console, but no equivalent exists yet
for targets using QEMUDriver. Resolving that may be a bigger
undertaking, so for now, let's provide a class method
get_qemu_base_args, that returns the list of arguments sans QMP parts.

Users can then initialize labgrid as usual and call the function to
get the command line and start Qemu for interactive use without having
to duplicate the labgrid environment parsing as in 1.

- what do you use the feature for? Start Qemu interactively with same environment yaml used for testing
- how does labgrid benefit as a testing library from the feature? makes debugging failing tests easier
- how did you verify the feature works? I verified tests work and that calling the function starts a usable qemu

Checklist

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • Add a section on how to use the feature to doc/usage.rst
  • Add a section on how to use the feature to doc/development.rst
  • PR has been tested
  • Man pages have been regenerated

THis is rebased on #1211 to avoid a conflict.

@codecov
Copy link

codecov bot commented Jun 16, 2023

Codecov Report

Patch coverage: 60.3% and project coverage change: -0.1 ⚠️

Comparison is base (4a7c688) 62.9% compared to head (318e778) 62.9%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1212     +/-   ##
========================================
- Coverage    62.9%   62.9%   -0.1%     
========================================
  Files         159     159             
  Lines       11737   11746      +9     
========================================
+ Hits         7388    7393      +5     
- Misses       4349    4353      +4     
Impacted Files Coverage Δ
labgrid/driver/qemudriver.py 69.6% <60.3%> (+0.5%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@a3f a3f force-pushed the afa/run-qemu-interactive branch from 280a1be to e1fb3b6 Compare June 16, 2023 23:34
@a3f a3f changed the title qemudriver: provide run_qemu_interactive method qemudriver: export get_qemu_base_args method Jun 16, 2023
a3f added 3 commits June 19, 2023 08:00
This introduces no functional change, but makes it easier to split the
function into two in a follow-up commit:

  - one function to just compute the base command line
  - one function to actually store the command line into the object
    along with QMP-specific options

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Next commit will factor command line computation out of on_activate to
make it available for use with an interactively started Qemu. Prepare
for this by collecting everything that is not applicable to non-QMP
usage at the end of the function.

No functional change intended.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
For debugging, it can be useful to start an interactive session for the
user to access the DUT console. This is possible with real targets
through labgrid-client -s STATE console, but no equivalent exists yet
for targets using QEMUDriver. Resolving that may be a bigger
undertaking, so for now, let's provide a class method
get_qemu_base_args, that returns the list of arguments sans QMP parts.

Users can then initialize labgrid as usual and call the function to
get the command line and start Qemu for interactive use without having
to duplicate the labgrid environment parsing as in [1].

[1]: https://github.com/barebox/barebox/blob/v2023.05.0/test/emulate.pl#L226

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Copy link
Member

@Emantor Emantor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how this is used in the supplied test script? The only caller introduced in the commits is the old location you extracted the function from. Please explain how this works in conjunction with your perl script.

@a3f
Copy link
Contributor Author

a3f commented Jun 19, 2023

barebox test/emulate.pl duplicates the YAML parsing to be able to run Qemu interactively. This PR is preparation for using existing labgrid infrastructure for that. See here for how it's used: https://lore.barebox.org/barebox/20230619095240.4168216-4-a.fatoum@pengutronix.de/T/#m6e6deb363efc855b0ec77d46d8f2ba6c0f67ff21

My first PR attempt just started QEMU directly. I am open to implement it either way.

@a3f
Copy link
Contributor Author

a3f commented Jun 22, 2023

Is this ok? Should I split this PR up, so the non-functional changes can be merged first and I can submti PRs for other Qemu related things?

Copy link
Member

@jluebbe jluebbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

104223c should not be needed with 552a42c, or am I missing something?

Looks OK otherwise. Later, we might want to expose this string via the export functionality.

@a3f
Copy link
Contributor Author

a3f commented Jun 22, 2023

104223c should not be needed with 552a42c, or am I missing something?

@jluebbe Yes, I had rebased on #1211 to avoid the conflict, but I see now that it's not required.

Looks OK otherwise. Later, we might want to expose this string via the export functionality.

What's that?

@jluebbe
Copy link
Member

jluebbe commented Jun 22, 2023

@jluebbe Yes, I had rebased on #1211 to avoid the conflict, but I see now that it's not required.

Looks OK otherwise. Later, we might want to expose this string via the export functionality.

What's that?

See 70c66cc. It's built into labgrid-client so it won't work for your use-case yet.

@a3f a3f force-pushed the afa/run-qemu-interactive branch from e1fb3b6 to 318e778 Compare June 22, 2023 15:54
@Emantor Emantor requested review from jluebbe and Emantor August 1, 2023 06:02
@Emantor Emantor added the Ready for review PRs which should be Rereviewed after Changes Requested label Aug 1, 2023
@Emantor Emantor merged commit 2dcc631 into labgrid-project:master Sep 6, 2023
7 of 9 checks passed
@a3f a3f deleted the afa/run-qemu-interactive branch September 6, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review PRs which should be Rereviewed after Changes Requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants