Fix and validate manual (De)Serialize implementations with serde_json

[likebreath]

Summary of the PR

The 'serde_json' crate has been commonly used to (de)serialize Rust data structures in the format of JSON, such as Cloud Hyervisor uses this crate for its live migration support. This patch adds a set of unit tests to ensure our manual (De)Serialize implementations works with the 'serde_json' crate.

The new Deserializer implementation (https://github.com/rust-vmm/kvm-bindings/pull/107) does not work with the 'serde_json' crate, particularly serde_json::{to_string, from_str}. The new implementation always expects byte array for deserilaiztion. Meanwhile, the 'serde_json' crate always (de)serializes String as Vector [1][2] which is treated as sequence (e.g. not 'byte array') by default for (de)serialization [3].

Reverting #107 seems to be the simplest solution to fix this issue. Suggestions are welcome.

[1] https://github.com/serde-rs/json/blob/cc7a1608c9bb7736c884926e016421af41a1ebe7/src/ser.rs#L2168-L2175 [2] https://github.com/serde-rs/json/blob/cc7a1608c9bb7736c884926e016421af41a1ebe7/src/de.rs#L30-L39 [3] https://serde.rs/data-model.html#types

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

• [x] All commits in this PR have Signed-Off-By trailers (with git commit -s), and the commit message has max 60 characters for the summary and max 75 characters for each description line.
• [x] All added/changed functionality has a corresponding unit/integration test.
• [x] All added/changed public-facing functionality has entries in the "Upcoming Release" section of CHANGELOG.md (if no such section exists, please create one).
• [x] Any newly added unsafe code is properly documented.

[likebreath]

@roypat Can you please take a look? Thank you.

[roypat]

@roypat Can you please take a look? Thank you.

Hi @likebreath,

sorry, I was out sick last week :( (and yesterday was public holiday in the UK) I will have a look today or tomorrow!

[roypat]

Alright, I had a look. Definitely +1 on adding the serde_json tests, sorry for missing this the last time :(

As for reverting #107, Firecracker kinda needs it for performance reasons (our deserialization is considered a hot path). Could you try the below diff instead? It makes the serde_json tests pass locally for me, while keeping #107 in place:

diff --git a/src/serialize.rs b/src/serialize.rs
index 16cb1d7..75c6851 100644
--- a/src/serialize.rs
+++ b/src/serialize.rs
@@ -42,6 +42,18 @@ macro_rules! serde_impls {
                             backing[..limit].copy_from_slice(&bytes[..limit]);
                             Ok(backing)
                         }
+
+                        fn visit_seq<A: serde::de::SeqAccess<'a>>(self, mut seq: A) -> Result<Self::Value, A::Error> {
+                            let mut backing = [0u8; std::mem::size_of::<$typ>()];
+
+                            for backing_byte in &mut backing {
+                                let Some(byte) = seq.next_element()? else { break };
+
+                                *backing_byte = byte;
+                            }
+
+                            Ok(backing)
+                        }
                     }

                     let backing = deserializer.deserialize_bytes(BytesVisitor)?;

[likebreath]

@roypat Thanks for looking into it and sharing a better fix. I can verify it resolves the issue with serde_json.

This PR has been updated to include your fix. Please take a look and let me know if you have any further comments.

[roypat]

I think we can probably wrap this into a patch release, since its a bug fix. @likebreath do you have time to prepare a release PR? Then Rob and me can probably approve and get it published today :)