Skip to content

Conversation

Manciukic
Copy link
Contributor

Changes

  • Generate virtio device ids with bindgen
  • simplify code to perform a VMM action on a device as a result of an API request
        if let Some(virtio_device) = self
            .device_manager
            .get_virtio_device(TYPE_BALLOON, BALLOON_DEV_ID)
        {
            let config = virtio_device
                .lock()
                .expect("Poisoned lock")
                .as_mut_any()
                .downcast_mut::<Balloon>()
                .unwrap()
                .config();

            Ok(config)
        } else {
            Err(BalloonError::DeviceNotFound)
        }

becomes

        self.device_manager
            .with_virtio_device_with_id(
                virtio_ids::VIRTIO_ID_BALLOON,
                BALLOON_DEV_ID,
                |dev: &mut Balloon| dev.config(),
            )
            .map_err(VmmError::FindDeviceError)

by extending the existing with_virtio_device_with_id implementation (now all accesses are using this pattern).

  • cleanup unused error in balloon and fixup a particular error message

Reason

I will be adding a bunch of these with_virtio_device_with_id for the virtio-mem device and this makes it simpler rather than writing the old long if branches.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Aug 15, 2025

Codecov Report

❌ Patch coverage is 35.61644% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.41%. Comparing base (5e94d12) to head (3e99b29).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/vmm/src/lib.rs 0.00% 27 Missing ⚠️
src/vmm/src/device_manager/mod.rs 0.00% 14 Missing ⚠️
src/vmm/src/rpc_interface.rs 0.00% 4 Missing ⚠️
src/vmm/src/devices/virtio/balloon/device.rs 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5387      +/-   ##
==========================================
+ Coverage   82.25%   82.41%   +0.15%     
==========================================
  Files         266      266              
  Lines       30619    30571      -48     
==========================================
+ Hits        25185    25194       +9     
+ Misses       5434     5377      -57     
Flag Coverage Δ
5.10-c5n.metal 82.37% <35.61%> (+0.12%) ⬆️
5.10-m5n.metal 82.36% <35.61%> (+0.12%) ⬆️
5.10-m6a.metal 81.64% <35.61%> (+0.12%) ⬆️
5.10-m6g.metal 78.98% <35.61%> (+0.12%) ⬆️
5.10-m6i.metal 82.37% <35.61%> (+0.12%) ⬆️
5.10-m7a.metal-48xl 81.64% <35.61%> (?)
5.10-m7g.metal 78.98% <35.61%> (+0.12%) ⬆️
5.10-m7i.metal-24xl 82.34% <35.61%> (?)
5.10-m7i.metal-48xl 82.34% <35.61%> (?)
5.10-m8g.metal-24xl 78.98% <35.61%> (?)
5.10-m8g.metal-48xl 78.98% <35.61%> (?)
6.1-c5n.metal 82.41% <35.61%> (+0.12%) ⬆️
6.1-m5n.metal 82.41% <35.61%> (+0.12%) ⬆️
6.1-m6a.metal 81.70% <35.61%> (+0.13%) ⬆️
6.1-m6g.metal 78.98% <35.61%> (+0.11%) ⬆️
6.1-m6i.metal 82.41% <35.61%> (+0.12%) ⬆️
6.1-m7a.metal-48xl 81.69% <35.61%> (?)
6.1-m7g.metal 78.98% <35.61%> (+0.12%) ⬆️
6.1-m7i.metal-24xl 82.42% <35.61%> (?)
6.1-m7i.metal-48xl 82.43% <35.61%> (?)
6.1-m8g.metal-24xl 78.98% <35.61%> (?)
6.1-m8g.metal-48xl 78.98% <35.61%> (?)

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.

@Manciukic Manciukic changed the title Refactor/with virtio device fn refactor: Simplify VMM code to perform API action on device Aug 15, 2025
@Manciukic Manciukic force-pushed the refactor/with-virtio-device-fn branch from 10458b5 to 5142dd1 Compare August 18, 2025 10:38
@Manciukic Manciukic marked this pull request as ready for review August 18, 2025 11:17
@Manciukic Manciukic added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Aug 18, 2025
@Manciukic Manciukic requested review from bchalios and roypat August 18, 2025 11:17
roypat
roypat previously approved these changes Aug 18, 2025
@roypat roypat enabled auto-merge (rebase) August 18, 2025 12:13
Currently, the device ids (TYPE_*) are hardcoded in various places. With
this change, they are generated from linux headers.

Signed-off-by: Riccardo Mancini <[email protected]>
Make the with_virtio_device_with_id function more generic and introduce
its variant with error handling try_with_virtio_device_with_id. Then use
these new functions in vmm.rs whenever we need to perform an action on a
device.

To simplify the code, I also moved some balloon device error handling
back to the balloon device code and refactored it a little bit.

Signed-off-by: Riccardo Mancini <[email protected]>
Remove some error variants that were not used in the code.

Signed-off-by: Riccardo Mancini <[email protected]>
The balloon device always returns "Amount of pages requested cannot fit
in u32" even if it fails due to the guest memory check.

Reword the error to make it more clear.

Signed-off-by: Riccardo Mancini <[email protected]>
As the caller of latest_stats() always clones the object, just derive
Copy on it and return the copy rather than a reference.

Signed-off-by: Riccardo Mancini <[email protected]>
By defining an associated function to the trait, we can simplify the
logic to execute code on a specific device from the VMM, while also
statically guaranteeing we are passing the right constant.

The downside is that we need both a sized and a dyn implementation for
the function. To ensure devices implement them correctly, a utility
macro is provided. We cannot do it as a default trait impl as the const_
variant is only defined on Sized.

Signed-off-by: Riccardo Mancini <[email protected]>
Copy link
Contributor

@bchalios bchalios left a comment

Choose a reason for hiding this comment

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

LGTM! Cool stuff

@roypat roypat merged commit acb79a2 into firecracker-microvm:main Aug 19, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Awaiting review Indicates that a pull request is ready to be reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants