Make Versionize and serde_bincode compatible and split Versionize/Snapshot errors. #1720
Conversation
src/versionize/src/primitives.rs
Outdated
|
|
||
| let restored_state: TestCompatibility = | ||
| bincode::deserialize_from(snapshot_mem.as_slice()).unwrap(); | ||
| assert_eq!(test_struct._string, restored_state._string); |
There was a problem hiding this comment.
Is it possible to derive PartialEq for TestCompatibility so we can have a single statement like assert_eq!(test_struct, restored_state);?
There was a problem hiding this comment.
Having an expanded assertion simplifies debugging by showing which primitive is broken.
There was a problem hiding this comment.
nit: I like the PartialEq solution better too; if something's broken, someone will look into it and figure it out eventually.
There was a problem hiding this comment.
After more thought on this I've also convinced myself:)
| } | ||
|
|
||
| #[derive(Debug, serde_derive::Deserialize, serde_derive::Serialize, Versionize)] | ||
| struct TestCompatibility { |
There was a problem hiding this comment.
I guess we can also at least add u128, i128, a tuple, and a tuple struct.
There was a problem hiding this comment.
Those are not yet implemented. i128 does not seem to be needed right away. I will add an issue for tuple.
| let v: Vec<#array_type_token> = <Vec<#array_type_token> as Versionize>::deserialize(&mut reader, version_map, app_version)?; | ||
| vec_to_arr_func!(transform_vec, #array_type_token, #array_len); | ||
| transform_vec(v) | ||
| let mut array = [#array_type_token::default() ; #array_len]; |
There was a problem hiding this comment.
I don't think we should rely on the assumption that array members always implement Default even though the alternatives are more verbose/complex :( One option coming to mind is to rely on macros somewhere (i.e. the primitives file) to implement Versionize for [T; 0] to [T; 32] (which seems to be standard practice when adding trait implementation for arrays, since Rust doesn't accept generics in terms of numerical values). I think the same applies to tuples, up to a certain cardinality.
There was a problem hiding this comment.
I am fine with the current limitation (Default trait) and the simple implementation as it handles any array size without having to generate impls for Versionize for [T; 0] to [T; 32].
I'll try someting when I implement tuples.
There was a problem hiding this comment.
Cool, maybe we can mention this in the issue you referenced before as well.
src/versionize/src/primitives.rs
Outdated
|
|
||
| let restored_state: TestCompatibility = | ||
| bincode::deserialize_from(snapshot_mem.as_slice()).unwrap(); | ||
| assert_eq!(test_struct._string, restored_state._string); |
There was a problem hiding this comment.
nit: I like the PartialEq solution better too; if something's broken, someone will look into it and figure it out eventually.
|
@aghecenco @alexandruag I've updated the PR please take a look. |
- Implemented VersionizeResult. - Split versionize::Error into snapshot::Error and versionize:Error - Implement bincode compatible array serialization. - Add test for bincode compatibility. Signed-off-by: Andrei Sandu <[email protected]>
Reason for This PR
Fixes #1718, #1707.
Description of Changes
Described in commit message.
rust-vmm.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).unsafecode is properly documented.firecracker/swagger.yaml.CHANGELOG.md.