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

Rationalize HTTP_WRONG_SETTING_DIRECTION #2814

Merged
merged 3 commits into from Jun 27, 2019

Conversation

LPardue
Copy link
Member

@LPardue LPardue commented Jun 19, 2019

This fixes #2810 in a different way by subtituting the error code and not changing the requirements related to NUM_PLACEHOLDERS.

Closes #2811.

@ianswett ianswett added the -http label Jun 19, 2019
Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

Looks good!

HTTP_SETTINGS_ERROR (0x01):
: An endpoint detected an error with a SETTINGS frame: a duplicate setting was
detected, a client-only setting was sent by a server, or a server-only setting
by a client.
Copy link
Contributor

Choose a reason for hiding this comment

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

The suggestion to move HTTP_GENERAL_PROTOCOL_ERROR to 0x01 might be worth taking at the same time, since people are going to change which error to use for these anyway. (Also, I just noticed that GPE is missing from the IANA table.)

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my initial intention but I thought it might make life harder. However, life is hard regardless so I'll try it out.

(Also, I just noticed that GPE is missing from the IANA table.)

You reminded me that I fixed this on the MALFORMED_FRAME PR. It makes sense to fix that here though while shuffling things around.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@@ -1902,7 +1903,7 @@ The entries in the following table are registered by this document.
| Name | Code | Description | Specification |
| ----------------------------------- | ---------- | ---------------------------------------- | ---------------------- |
| HTTP_NO_ERROR | 0x0000 | No error | {{http-error-codes}} |
| HTTP_WRONG_SETTING_DIRECTION | 0x0001 | Setting sent in wrong direction | {{http-error-codes}} |
| HTTP_SETTINGS_ERROR | 0x0001 | Setting duplication or wrong direction | {{http-error-codes}} |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be more general -- SETTINGS frame contained invalid values, or something like that. It's anything that could go wrong with that frame's payload.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed.

: Peer violated protocol requirements in a way which doesn't match a more
specific error code, or endpoint declines to use the more specific error code.
HTTP_SETTINGS_ERROR (0x00FF):
: An endpoint detected an error with a SETTINGS frame: a duplicate setting was
Copy link
Member

@kazuho kazuho Jun 20, 2019

Choose a reason for hiding this comment

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

Maybe "detected an error in the values found in the payload of a SETTINGS frame"?

I suggest this assuming that HTTP_MALFORMED_FRAME is the error code to be used when seeing a badly-formatted SETTINGS frame (for example a SETTINGS frame carrying one byte of data: 0xff).

Copy link
Member Author

Choose a reason for hiding this comment

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

I added you suggestion for payload.

The ultimate goal is to replace MALFORMED_FRAME with FRAME_SIZE_ERROR. That should cover the case you mention. At that point in time we an add some text to suggest which type of error code should be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rationalise HTTP_WRONG_SETTING_DIRECTION error
4 participants