Fix Frame Size Issue #690
Merged
Fix Frame Size Issue #690
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
willmeister
approved these changes
Dec 3, 2018
|
|
||
| if expected_frame_mac != frame_mac do | ||
| {:error, "Failed to match frame ingress mac"} | ||
| if byte_size(frame_rest) < frame_size + frame_padding_bytes + 16 do |
There was a problem hiding this comment.
What's the 16 for here? Can we make it into a descriptive constant?
| %{frame_secrets | ingress_mac: ingress_mac, decoder_stream: decoder_stream} | ||
| } | ||
| frame_enc::binary-size(frame_size), | ||
| frame_padding::binary-size(frame_padding_bytes), |
There was a problem hiding this comment.
Should this just be frame_enc_with_padding::binary-size(frame_size + frame_padding_bytes)? Looks like they're pattern matched separately only to be concatenated below and never used individually.
|
I've noticed that binary isn't a dependency of ex_wire, could you include it? |
InoMurko
requested changes
Dec 6, 2018
| | {:error, | ||
| :insufficient_data | ||
| | :failed_to_match_header_ingress_mac | ||
| | :failed_to_match_frame_ingress_mac} | ||
| def unframe( |
There was a problem hiding this comment.
if you included frame parsing in the header, as in
def unframe(<<header_enc::binary-size(16), header_mac::binary-size(16), frame_rest::binary()>>, frame_secrets = %Secrets{
ingress_mac: ingress_mac,
decoder_stream: decoder_stream,
mac_encoder: mac_encoder,
mac_secret: mac_secret
})
def unframe(<<header_enc::binary-size(16), header_mac::binary-size(16), frame_rest::binary()>>, frame_secrets = %Secrets{
ingress_mac: ingress_mac,
decoder_stream: decoder_stream,
mac_encoder: mac_encoder,
mac_secret: mac_secret
}) do
...
end
def unframe(_, _), do: {:error, :insufficient_data}
You could avoid deep nested function.
When packets come in, sometimes they are incomplete. We previously handled packets that had a valid header but insufficient body size. This patch adds support for packets that don't even have complete headers. We queue the packet into queued data and wait for more data before processing. Address PR feedback
hayesgm
force-pushed
the
hayesgm/frame-size-issue
branch
from
48cc61c
to
00fdd6e
Compare
|
I addressed the PR feedback, ready for merge. |
InoMurko
approved these changes
Dec 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When packets come in, sometimes they are incomplete. We previously handled packets that had a valid header but insufficient body size. This patch adds support for packets that don't even have complete headers. We queue the packet into queued data and wait for more data before processing.