Rustify snapshot module #4691
Rustify snapshot module #4691
Conversation
| // The snapshot version we can handle | ||
| version: Version, | ||
| } | ||
| pub fn load<R: Read>(reader: &mut R) -> Result<Self, SnapshotError> { |
There was a problem hiding this comment.
This function should also do the check the the header's magic value is correct
src/vmm/src/snapshot/mod.rs
Outdated
| pub data: Data, | ||
| } | ||
|
|
||
| impl<Data: for<'a> Deserialize<'a>> Snapshot<Data> { | ||
| /// Helper function to deserialize an object from a reader | ||
| pub fn deserialize<T, O>(reader: &mut T) -> Result<O, SnapshotError> |
There was a problem hiding this comment.
This function should probably be free-standing, and not pub. Then you can reuse it inside SnapshotHdr::load as well
There was a problem hiding this comment.
By free standing I'm assuming you mean not have the deserialize and serialize functions as implementations of Snapshot so I moved them outside.
| } | ||
|
|
||
| /// Load a snapshot from a reader and validate its CRC | ||
| pub fn load<T, O>(reader: &mut T, snapshot_len: usize) -> Result<(O, Version), SnapshotError> | ||
| pub fn load<R: Read>(reader: &mut R, snapshot_len: usize) -> Result<Self, SnapshotError> |
There was a problem hiding this comment.
This function's implementation we can change to only do a single pass over the snapshot bytes, as well as not taking a snapshot_len: usize parameter, I think:
- Construct
crc_readerfrom the given reader, as is done now - Deserialize the
Snapshot<Data>object from this reader. Since thestruct Snapshotdoes not contain thecrc, andallow_trailing_bytesis set forbincode, this will simply leave the crc bytes unread. - at the end, store the crc from the
CrcReaderin some local variable. - Read the remaining 4 bytes in the input stream, and check that they match the computed CRC.
src/vmm/src/snapshot/mod.rs
Outdated
| ) -> Result<O, SnapshotError> | ||
| impl<Data: Serialize + Debug> Snapshot<Data> { | ||
| /// Helper function to serialize an object to a writer | ||
| pub fn serialize<T, O>(writer: &mut T, data: &O) -> Result<usize, SnapshotError> |
There was a problem hiding this comment.
just like deserialize, this can be a free standing function, non-pub, and be reused in SnapshotHdr
src/vmm/src/snapshot/mod.rs
Outdated
| .map_err(|err| SnapshotError::Serde(err.to_string()))?; | ||
|
|
||
| Ok(buffer.len()) | ||
| // bincode::serialize_into(writer, data).map_err(|err| SnapshotError::Serde(err.to_string())) |
There was a problem hiding this comment.
Why use a temporary buffer instead of just this commented out line? :o
|
Messaging to let you know I'm still working on this |
Hey, just wanted to check in if you're still looking into this or need help with anything :) |
|
Hi, yeah I still am. I'll push commits addressing your comments and a few other changes in the next couple days. |
Changes
...
Reason
...
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
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.