Refactor Snapshot Module

[gjkeller]

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.rs to 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 Reader with CrcReader and pass into load_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:

#[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 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.

[roypat]

Heya, thanks for picking this up, this is a great start! Our CI doesn't start running without one of us actually manually unblocking, so it's probably a quicker turnabout for you if you locally set up a rust toolchain via rustup. Even if you don't have a linux dev box, cargo's cross-target support for compile testing is pretty good, so you should be able to catch most problems that way (e.g. just doing cargo check --target x86_64-unknown-linux-gnu after rustup target add x86_64-unknown-linux-gnu).

I also had an early look and got some early feedback :)

• Please squash all of the commits into one
• You seem to be missing the definition of struct Snapshot<Data>, but you have two serialize functions defined
• Tying into the previous point, if you define struct Snapshot<Data> as having two fields of type SnapshotHdr and Data respectively, then you won't need to implement load/store on SnapshotHdr. You can simply deserialize the entire snapshot object from the given Read instance. The check of the magic bytes can be moved to load_with_verison_check [sic].
• I think you can elide a lot of the explicit type annotations on your let statements

[gjkeller]

Thanks for the feedback. I've gotten rustup set up, will be sure to run it more frequently to make sure build/lint/tests passes. Also, I do see the Git linting issues, good suggestion to squash all my commits once I get build/lint/tests passing and am ready for review. Are you also suggesting I squash @beamandala's commits in with my own? They said here that it would be okay for me to pick up the work, but I just wanted to check to see if it was appropriate to squash their commits with mine since they weren't signed off and may not have been passing.

As for the missing definitions, duplicate definitions, etc., I'm not sure what happened here -- I must have misused a Git command somewhere down the line, I will fix this. My plan was to have Snapshot's load internally call SnapshotHdr's load so that the logic behind handling headers, recognizing magic values, etc. is separated.

[roypat]

Are you also suggesting I squash @beamandala's commits in with my own? They said here that it would be okay for me to pick up the work, but I just wanted to check to see if it was appropriate to squash their commits with mine since they weren't signed off and may not have been passing.

Yes, this is fine, please just squash everything :)

My plan was to have Snapshot's load internally call SnapshotHdr's load so that the logic behind handling headers, recognizing magic values, etc. is separated.

I don't think separating it out is worth is that much, since it does make the code a bit more difficult to understand. If you really want to do the checks as part of deserialization of SnapshotHdr though, I think its cleaner to use a deserialize_with attribute on the version and magic fields

[roypat]

Hey, thanks again for your contribution! We've ended up implementing this ourselves in https://github.com/firecracker-microvm/firecracker/pull/5355, so I'll go ahead and close this