feat(s2n-quic-dc): Add Unix socket packet encoding/decoding #2840
Conversation
mehnazyunus
force-pushed
the
packet-serialization
branch
from
7d40947 to
50b9cdb
Compare
| } | ||
|
|
||
| let application_params = | ||
| ApplicationParams::new(max_datagram_size, &peer_flow_control_limits, &limits); |
There was a problem hiding this comment.
I think we should move this to s2n-quic-core so that we can make it harder to accidentally forget encoding/decoding a field in ApplicationParams (e.g., if we start making use of a new Limit, then I think the logic here wouldn't notice that).
| encoder.encode(&payload_len); | ||
|
|
||
| for &byte in payload { | ||
| encoder.encode(&byte); |
There was a problem hiding this comment.
I think you want to use https://docs.rs/s2n-codec/latest/s2n_codec/encoder/trait.Encoder.html#method.encode_with_len_prefix here, which I believe takes care of both length + buffer writing, likely much more efficiently than what you have here.
| application_params: &ApplicationParams, | ||
| payload: &[u8], | ||
| ) -> usize { | ||
| let mut size = 0; |
There was a problem hiding this comment.
Do we have other examples of this kind of manual computation in s2n-quic code? I'm wondering if we should just use https://docs.rs/s2n-codec/latest/s2n_codec/encoder/estimator/struct.EncoderLenEstimator.html to estimate the size by running the real encoder, or just always encode into relatively large buffers (e.g., a reused 64kb buffer).
| /// [ wire packet ] - bytes | ||
| /// | ||
| #[inline(always)] | ||
| pub fn encode( |
There was a problem hiding this comment.
You should be able to make this function generic over the encoder and get the encoding size for free by passing it an estimator. The reason the other pack types didn't do that in this repo is because the crypto copy avoidance interface wasn't really possible, or at least I didn't spend enough time trying to get it to work. In this case your encoding function is simple enough to be generic though.
| pub fn encode( | |
| pub fn encode<E: Encoder>( | |
| encoder: &mut E, |
quic/s2n-quic-core/src/dc.rs
Outdated
|
|
||
| match self.max_idle_timeout { | ||
| Some(timeout) => { | ||
| buffer.encode(&timeout.get()); |
There was a problem hiding this comment.
Not sure how much we care about compressing integers down but for consistency it's probably better to encode this as a varint since all the other fields are as well
There was a problem hiding this comment.
Converting NonZeroU32 to and from VarInt looks a little messy I think, not sure if there is a better way to do that.
3 participants
Release Summary:
Add packet encoding and decoding for packets sent over Unix domain sockets. This is required for reusing dcQUIC handshakes between application processes.
Resolved issues:
n/a
Description of changes:
Add packet encoding and decoding for packets sent over Unix domain sockets. This is required for reusing dcQUIC handshakes between application processes.
Call-outs:
Added consts for packet version and app param version in encoder.rs. Not sure if this should live elsewhere like
WireVersion.Testing:
Added unit tests for encoding/decoding.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.