-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Elimination of unneeded generics and lots of .map_err()
#5106
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
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5106 +/- ##
==========================================
+ Coverage 83.13% 83.19% +0.06%
==========================================
Files 249 249
Lines 26937 26885 -52
==========================================
- Hits 22393 22368 -25
+ Misses 4544 4517 -27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pb8o
reviewed
Mar 24, 2025
f8bdd1a
to
1400476
Compare
ShadowCurse
reviewed
Mar 24, 2025
8e8a5ca
to
a36bd7e
Compare
ShadowCurse
previously approved these changes
Mar 24, 2025
kalyazin
reviewed
Mar 25, 2025
kalyazin
previously approved these changes
Mar 25, 2025
In CI we always run unittests without `--features gdb`, but actually with the gdb feature they dont compile. Fix this with some cfgs. Signed-off-by: Patrick Roy <[email protected]>
There is absolutely no need for generics anywhere in this module, as the trait we were being generic over had only a single implementor. Signed-off-by: Patrick Roy <[email protected]>
This test is all about testing what happens when a vsock driver places virtio buffers close to / around the MMIO gap on x86_64 systems. Since this test thus doesn't really serve a purpose on aarch64, we can just cfg(target_arch = "x86_64") it, and reuse the constants from arch::x86_64 instead of redefining them again. fwiw, I have no idea what "bof" means. I'm also confused by this test passing on ARM in the first place, given that some of the comments indicate to me that it should fail if the buffers overlap the mmio gap (which doesn't exist on arm, so nothing should fail?). Signed-off-by: Patrick Roy <[email protected]>
Left-shifting by 20 to convert MiB to bytes is ubiquitous in our codebase, but probably somewhat arcane to people not familiar with low level stuff. Let's define a helper for the conversion. Signed-off-by: Patrick Roy <[email protected]>
The return value here was ignored anyway, so no need to map the unit () to VmmData::Empty. Signed-off-by: Patrick Roy <[email protected]>
This means that `create_vmm_and_vcpus` now does not use any variants of `StartMicrovmError` directly, and as a next step can be converted to return `VmmError` directly, which will eliminate all the `map_err` inside. Signed-off-by: Patrick Roy <[email protected]>
Eliminate a lot of .map_err(Internal) to make the code more readable. Signed-off-by: Patrick Roy <[email protected]>
When a variant wraps another error type, and this error type does not appear in any other variant, use thiserrors #[from] directive to implement a `From` impl. This allows eliminating almost all remaining `map_err`s from `create_vmm_and_vcpus`, as the `?` operator will automatically call .from() once (although it cannot do it for multiple layers of wrapping, which is why it was important to change the function to return VmmError). Signed-off-by: Patrick Roy <[email protected]>
This allows us to eliminate some more .map_err(Internal). Signed-off-by: Patrick Roy <[email protected]>
90e2e9b
to
52104ba
Compare
kalyazin
approved these changes
Mar 25, 2025
ShadowCurse
approved these changes
Mar 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A collection of cleanups I accumulated on a local branch while prototyping for #4522. Mainly
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
tools/devtool checkstyle
to verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md
.Runbook for Firecracker API changes.
integration tests.
TODO
.rust-vmm
.