Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify coalesced packet processing #1844

Closed
wants to merge 3 commits into from

Conversation

igorlord
Copy link
Contributor

@igorlord igorlord commented Oct 8, 2018

Addressing #1840

packets were received out-of-order in separate datagrams.
they were received as the payload of separate UDP datagrams. For example, if
one or more packets in a datagram cannot be decrypted (the keys are not yet
available, decryption failure, unknown type), the receiver MAY either discard or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

", decryption fails, or the packet is of an unknown type"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Applied the suggestion.

they were received as the payload of separate UDP datagrams. For example, if
one or more packets in a datagram cannot be decrypted (the keys are not yet
available, decryption failure, unknown type), the receiver MAY either discard or
buffer for packet for later processing and MUST attempt to process the remaining
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for packet -> the packet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

@mikkelfj mikkelfj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no more long headers are expected, it would seem reasonable to drop the packet.

@igorlord
Copy link
Contributor Author

igorlord commented Oct 9, 2018

Long headers are useful for PMTUD to validate (and route) PTB ICMP messages. So we discussed in Seattle the idea of prepending long header packets to PMTUD Probe packets.

packets MAY either be discarded or buffered for later processing, just as if the
packets were received out-of-order in separate datagrams.
they were received as the payload of separate UDP datagrams. For example, if
decryption fails or the packet is of an unknown type, the receiver MAY either
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to lose the case this text was originally added to address, which is that you don't have the keys to process this packet type yet. While these are non-limiting examples, that's probably a case worth calling out explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Updated the PR to mention this case specifically.

packets were received out-of-order in separate datagrams.
they were received as the payload of separate UDP datagrams. For example, if
decryption fails (the keys are not yet available or any other reason) or the
packet is of an unknown type, the receiver MAY either discard or buffer the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the packet" does not make sense in this context. How about "If one or more packets in a datagram cannot be processed yet, because of decryption failure or any other reason, the receiver MAY either discard or buffer the packets for later processing. The receiver MUST still attempt to process the remaining packets in the UDP datagram."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good. Want to port that over to #1857?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would omit "yet". "Yet" implies knowing that it will be possible to decrypt the packet in the future. It is hard to know the future.

@martinthomson
Copy link
Member

Merged by hand.

@igorlord igorlord deleted the clarify-coalesced-packets branch December 13, 2018 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants