Skip to content

krun: Parse libkrun flavor indicated in KRUN_VM_FILE #1754

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

Merged
merged 3 commits into from
May 22, 2025

Conversation

tylerfanelli
Copy link
Member

@tylerfanelli tylerfanelli commented May 21, 2025

To support other libkrun flavors in the future, parse which flavor (and by extension, which handle) should be used before making any libkrun API calls past libkrun_create_ctx. The correct handle to be used can be determined before any other subsequent API calls.

cc @slp

Summary by Sourcery

Parse the KRUN_VM_FILE early to detect the requested libkrun flavor and select the appropriate handle before making further libkrun calls.

Enhancements:

  • Extract libkrun_read_vm_config to separate JSON parsing of the VM configuration file.
  • Introduce libkrun_configure_flavor to switch between standard and SEV libkrun based on the config file or presence of KRUN_SEV_FILE.
  • Update libkrun_exec to invoke the new config parsing and flavor-selection steps prior to VM configuration.

Copy link

sourcery-ai bot commented May 21, 2025

Reviewer's Guide

This PR refactors VM configuration handling in krun to support multiple libkrun flavors by splitting VM config parsing from flavor selection and wiring the new steps into the execution path.

File-Level Changes

Change Details Files
Extract separate VM config reader
  • Introduce new libkrun_read_vm_config function
  • Move file-reading, JSON parsing and early exit logic into it
  • Adjust parse_json_file call and cleanup variables
src/libcrun/handlers/krun.c
Refactor existing VM configurator to take parsed tree
  • Change libkrun_configure_vm signature to accept yajl_val pointer
  • Add check for NULL config_tree and return early
  • Update JSON field lookups to use dereferenced tree
src/libcrun/handlers/krun.c
Add flavor detection and selection
  • New libkrun_configure_flavor reads “flavor” field and KRUN_SEV_FILE
  • Determine whether to use SEV or vanilla handle/context
  • Set kconf->handle, ctx_id and sev flag accordingly
src/libcrun/handlers/krun.c
Wire new config steps into exec path
  • Remove inline SEV detection block from libkrun_exec
  • Call libkrun_read_vm_config then libkrun_configure_flavor
  • Pass config_tree into libkrun_configure_vm and assign handle/ctx_id from kconf
src/libcrun/handlers/krun.c

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @tylerfanelli - I've reviewed your changes - here's some feedback:

  • Ensure that the parsed YAJL tree (config_tree) is freed (e.g., via yajl_tree_free) after use to avoid memory leaks.
  • Avoid calling exit() in libkrun_configure_flavor—return an error code instead to keep error handling consistent and non-terminating within library functions.
  • Extract the literal "sev" into a named constant (or enum) to avoid magic strings and make adding future flavors easier.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@tylerfanelli tylerfanelli force-pushed the krun-vm-config-flavor branch 2 times, most recently from 57ee8cc to ad3fbc9 Compare May 21, 2025 02:05
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@tylerfanelli tylerfanelli force-pushed the krun-vm-config-flavor branch 2 times, most recently from e7876e8 to 0a4c757 Compare May 21, 2025 02:12
@@ -206,23 +262,18 @@ libkrun_exec (void *cookie, libcrun_container_t *container, const char *pathname
cpu_set_t set;
libcrun_error_t err;
bool configured = false;
yajl_val config_tree = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

when do we clean it up (yajl_tree_free (...))?

We exit early on errors so we don't really care in this case, but could we release it before krun_start_enter?

Copy link
Member Author

@tylerfanelli tylerfanelli May 22, 2025

Choose a reason for hiding this comment

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

Thanks for pointing that out, I never did free config_tree. I've modified 13fcca9 to free config_tree before krun_start_enter.

To determine the libkrun handle to use, the container may embed a field
"flavor" in the KRUN_VM_FILE. To prepare for this, pre-parse the krun VM
config when exec'ing.

Signed-off-by: Tyler Fanelli <[email protected]>
The krun VM config was parsed before the VM was configured. Use the
pre-parsed config tree already available instead of parsing it once
more.

Signed-off-by: Tyler Fanelli <[email protected]>
@tylerfanelli tylerfanelli force-pushed the krun-vm-config-flavor branch 3 times, most recently from 3f716cc to 5ca3fa7 Compare May 22, 2025 02:10
To support other krun flavors in the future, parse which flavor (and by
extension, which handle) should be used before making any libkrun API
calls past libkrun_create_ctx. The correct handle to be used can be
determined before any other subsequent API calls.

Signed-off-by: Tyler Fanelli <[email protected]>
@tylerfanelli tylerfanelli force-pushed the krun-vm-config-flavor branch from 5ca3fa7 to 379524f Compare May 22, 2025 02:15
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@slp slp left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe merged commit 735014f into containers:main May 22, 2025
44 checks passed
@tylerfanelli tylerfanelli deleted the krun-vm-config-flavor branch May 29, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants