Add serde derive macros where applicable #43
Conversation
|
Hi @daviessm , I like it, but I would as you proposed (and did) put it behind a non default feature flag. Greets |
| @@ -21,6 +21,7 @@ pub const IPV6_MAX_NUM_HEADER_EXTENSIONS: usize = 12; | |||
| /// * Host Identity Protocol | |||
| /// * Shim6 Protocol | |||
| #[derive(Clone)] | |||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |||
There was a problem hiding this comment.
I think a manual implementation of Serialize & Deserialize is needed for Ipv6RawExtensionHeader so that only the part of the payload buffer that is actually filled is serialized and/or deserialized. header_length defines the amount of data present in the payload_buffer. Additionally I would skip adding the header_length itself as it is implicitly encoded by the length of the payload_buffer.
| @@ -70,9 +71,16 @@ pub struct TcpHeader { | |||
| ///the URG control bit set. | |||
| pub urgent_pointer: u16, | |||
| ///Buffer containing the options of the header (note that the data_offset defines the actual length). Use the options() method if you want to get a slice that has the actual length of the options. | |||
| #[cfg_attr(feature = "serde", serde(skip), serde(default = "default_options_buffer"))] | |||
There was a problem hiding this comment.
I think a manual implementation of Serialize & Deserialize is needed for TcpHeader as well. Only the parts of the options_buffer that is actually filled should be serialized and/or deserialized. The length of actually filled data in the options_buffer is defined by the _data_offset field. The _data_offset field itself can be skipped when serializing as it is implicitly encoded in the length of the options_buffer.
We could also think about instead serializing the decoded options if that is possible. But that is maybe overkill.
| options_len: u8, | ||
| #[cfg_attr(feature = "serde", serde(skip), serde(default = "default_options_buffer"))] | ||
| options_buffer: [u8;40] |
There was a problem hiding this comment.
Same as with the TcpHeader & Ipv6RawExtension I would implement Serialize & Deserialize manually as options buffer is not required to be fully filled (same comments from the other two types also apply here).
|
Sorry I forgot: Thanks for the pull request 👍 . |
Codecov ReportBase: 99.63% // Head: 99.58% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #43 +/- ##
==========================================
- Coverage 99.63% 99.58% -0.06%
==========================================
Files 51 51
Lines 21898 21910 +12
==========================================
Hits 21818 21818
- Misses 80 92 +12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Hi @JulianSchmid, thanks for the review! I implemented this because I thought I needed it in a project but ended up writing a custom serializer in my code instead so I won't have any time to work on this any more. I'd be happy for this to be closed or taken over by someone else, whatever works best for you. |
|
Hi @daviessm , then I would take over the PR and implement it. |
|
I moved to branch to this repo and opened up a new PR #45 |
I had a requirement to serialize packets. What would you think of adding
serdesupport to the package, behind a feature flag?