firecracker
firecracker copied to clipboard
Refactor Snapshot Module
Draft PR as I finish the migration to new API and run linting/tests/build.
Changes
- Forked and pulled latest changes to @beamandala's fork of Firecracker associated with draft PR #4691
- Refactored
snapshot/mod.rsto reflect structure noted in #4523 and changes requested in draft PR #4691:- Specifically, changed the way that load was done for Snapshots so that the snapshot file is only read once (wrap
ReaderwithCrcReaderand pass intoload_unchecked) - Implemented some of my own changes to ensure it is clear which methods should be accessed by users of this API, but kept the general aforementioned structure:
- Specifically, changed the way that load was done for Snapshots so that the snapshot file is only read once (wrap
#[derive(Serialize, Deserialize)]
pub struct SnapshotHeader {
magic: u64,
version: Version
}
impl SnapshotHeader {
fn load<R: Read>(reader: &mut R) -> Result<Self> { ... }
fn store<W: Write>(writer: &mut W) -> Result<Self> { ... }
}
#[derive(Serialize, Deserialize)]
pub struct Snapshot<Data> {
header: SnapshotHeader,
data: Data
}
impl<Data: Deserialize> Snapshot<Data> {
fn load_unchecked<R: Read>(reader: &mut R) -> Result<Self> { ... }
fn load<R: Read>(reader: &mut R) -> Result<Self> { ... }
}
impl<Data: Serialize> Snapshot<Data> {
fn save<W: Write>(&self, writer: &mut W) -> Result<usize> { ... }
fn save_with_crc<W: Write>(&self, writer: &mut W) -> Result<usize> { ... }
}
- Removed load_with_version_check and associated test as it was unused
- Closes #4523, #4691
Reason
I took on @beamandala's draft PR with their permission to finish up the work. These changes were made so that the API would be easier to use / fall in line with general Rust conventions.
Instead of doing the following:
let snap = Snapshot::new(Version::new(1,0,0));
let (vm_state, ver) = snap.load(&mut reader, len)?;
// …or…
snap.save(&mut writer, &vm_state)?;
Which allows for incomplete variants of Snapshot (snapshots without data), the following is now done:
let snap: Snapshot<MyVmState> = Snapshot::load(&mut reader)?;
let ver = snap.version();
let state = snap.data;
// ...or...
let snap = Snapshot::new(Version::new(1,0,0), vm_state);
snap.save(&mut writer)?;
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
- [ X ] I have read and understand CONTRIBUTING.md.
- [ ] I have run
tools/devtool checkstyleto 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.