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
Simplify read_config #4458
base: main
Are you sure you want to change the base?
Simplify read_config #4458
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4458 +/- ##
==========================================
- Coverage 82.14% 82.12% -0.02%
==========================================
Files 255 255
Lines 31285 31262 -23
==========================================
- Hits 25700 25675 -25
- Misses 5585 5587 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall everything looks good, but please update commit messages (otherwise CI will be angry). Also can you update the write_config
as well while you are at it?
7a47fff
to
971bdab
Compare
However, I'm wondering if these functions should be reporting errors instead of just not reading/writing. Isn't there a risk from leaving |
Don't think returning errors makes a lot of sense here. There are 2 paths: panic and abort the process, or log the error and continue execution. In case of |
e0cc36f
to
e54fba3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay @kornelski @ShadowCurse,
I don't think there's any danger from failed reads/writes. At least, not on the host side. If the guest kernel driver is not working properly it might fail, but that's the guest's problem. We do the right thing.
LGTM, apart from the commit messages. Would you mind adding a bit of more info on what each commit does and why? Also, really a nit, but we typically have commit titles like:
virtio: simplify read_config for VirtIO devices
@kornelski Also please fix compilations/stylistics errors from here: https://buildkite.com/firecracker/firecracker-pr/builds/9248#_ |
e55297a
to
76a7c00
Compare
Hi @kornelski, could you fix the style issues in the PR so we can merge this? |
Rust doesn't guarantee struct layout by default, so `repr(C)` or `repr(transparent)` are necessary for safety. Signed-off-by: Kornel <kornel@cloudflare.com>
From: Kornel <kornel@cloudflare.com> `io::Write` has higher overhead and unnecessary error handling. Slices can be copied without it. Signed-off-by: Kornel <kornel@cloudflare.com> Signed-off-by: Patrick Roy <roypat@amazon.co.uk> Co-authored-by: Patrick Roy <roypat@amazon.co.uk>
Hi @kornelski, |
Changes
read_config
doesn't have to useio::Write
to just copy bytes in memory.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
][3].