pass ping and rsv tests #27
Conversation
| @@ -81,8 +81,15 @@ struct WSFrameParser { | |||
|
|
|||
| private mutating func parseOpCode(bytes: UnsafePointer<UInt8>, from: Int) -> (WebSocketError?, Int) { | |||
| let byte = bytes[from] | |||
| frame.finalFrame = byte & 0x80 != 0 | |||
| // 0x.. is the hexidecimal representation of the bits you would like to check | |||
|
|
||
| // Check RSV is equal to 0 |
There was a problem hiding this comment.
What does RSV stand for - can we add that info somewhere?
There was a problem hiding this comment.
RSV us short for reserved. I'll add that into the comment
|
|
||
| // Check RSV is equal to 0 | ||
| let rsv = (byte & 0x70) >> 4 |
There was a problem hiding this comment.
Why do we need to shift?
There was a problem hiding this comment.
so from the first byte, rsv bits are at 01110000. The shift makes the int[8] value of rsv represent 0-7 instead of 0, 16, 32, 48, 64. In our case since we are just ensuring they are 0 this doesn't matter but seemed sensible if we are RSV in future.
There was a problem hiding this comment.
I see. I'm not sure shifting is meaningful really - if we ever need to support RSV bits in future we can just mask them as needed. We're only ever going to refer to them as part of a UInt8 byte including the opcode, aren't we?
| @@ -19,6 +19,9 @@ import Foundation | |||
| /// An error enum used when throwing errors within KituraWebSocket. | |||
| public enum WebSocketError: Error { | |||
|
|
|||
| /// An invalid RSV was received in a WebSocket frame | |||
| case invalidRSV(UInt8) | |||
There was a problem hiding this comment.
This is a breaking change so will need a major version (switches have to be exhaustive)
There was a problem hiding this comment.
I could refactor the code so it returns an invalid opcode since currently the rsv values are being passed around as part of that?
There was a problem hiding this comment.
Technically the RSV bits aren't part of the opcode so you did the right thing, but... ugh this is annoying.
There was a problem hiding this comment.
They kind of are with how we are currently treating them. Currently If you send an op code of 25 it will send a ping with rsv = 7
There was a problem hiding this comment.
Perhaps that's one of the other testcase failures!
|
Have added a test for RSV and removed the RSV error so it is non-breaking. We could change the extension to WebSocketError so it gives different text if opcode is wrong or rsv is wrong? |
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
==========================================
- Coverage 90.44% 90.22% -0.22%
==========================================
Files 9 9
Lines 471 481 +10
==========================================
+ Hits 426 434 +8
- Misses 45 47 +2
Continue to review full report at Codecov.
|
This pull request if to make our websockets server pass Autobahn tests.
These changes make websockets pass:
Case 2.5 (oversized ping)
Case 3.* (Reserved bits are == 0)