From 4b4e487486a5b606ee90772203429223e8d3a7a2 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 12 Dec 2017 10:48:17 +0700 Subject: [PATCH] remove the error return value from wire.Frame.MinLength No functional change expected. The error was only non-nil if some required values for the STOP_WAITING frame were not set. It should be sufficient to throw an error when attempting to write an invalid STOP_WAITING frame. --- internal/wire/ack_frame.go | 6 +++--- internal/wire/ack_frame_legacy.go | 4 ++-- internal/wire/blocked_frame.go | 6 +++--- internal/wire/connection_close_frame.go | 6 +++--- internal/wire/frame.go | 2 +- internal/wire/goaway_frame.go | 4 ++-- internal/wire/max_data_frame.go | 6 +++--- internal/wire/max_stream_data_frame.go | 6 +++--- internal/wire/ping_frame.go | 4 ++-- internal/wire/rst_stream_frame.go | 6 +++--- internal/wire/stop_waiting_frame.go | 10 ++-------- internal/wire/stop_waiting_frame_test.go | 8 -------- internal/wire/stream_blocked_frame.go | 6 +++--- internal/wire/stream_frame.go | 4 ++-- internal/wire/stream_frame_legacy.go | 4 ++-- internal/wire/stream_frame_legacy_test.go | 8 ++++---- packet_packer.go | 16 +++------------- packet_packer_test.go | 18 ++++++------------ stream_framer.go | 8 +++----- stream_framer_test.go | 2 +- 20 files changed, 51 insertions(+), 83 deletions(-) diff --git a/internal/wire/ack_frame.go b/internal/wire/ack_frame.go index 5f0bc9750..4f37b0ade 100644 --- a/internal/wire/ack_frame.go +++ b/internal/wire/ack_frame.go @@ -139,7 +139,7 @@ func (f *AckFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) error } // MinLength of a written frame -func (f *AckFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *AckFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if !version.UsesIETFFrameFormat() { return f.minLengthLegacy(version) } @@ -157,7 +157,7 @@ func (f *AckFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount length += utils.VarIntLen(uint64(f.LargestAcked - lowestInFirstRange)) if !f.HasMissingRanges() { - return length, nil + return length } var lowest protocol.PacketNumber for i, ackRange := range f.AckRanges { @@ -169,7 +169,7 @@ func (f *AckFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount length += utils.VarIntLen(uint64(ackRange.Last - ackRange.First)) lowest = ackRange.First } - return length, nil + return length } // HasMissingRanges returns if this frame reports any missing packets diff --git a/internal/wire/ack_frame_legacy.go b/internal/wire/ack_frame_legacy.go index 3bef54034..42eaf24c5 100644 --- a/internal/wire/ack_frame_legacy.go +++ b/internal/wire/ack_frame_legacy.go @@ -308,7 +308,7 @@ func (f *AckFrame) writeLegacy(b *bytes.Buffer, _ protocol.VersionNumber) error return nil } -func (f *AckFrame) minLengthLegacy(_ protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *AckFrame) minLengthLegacy(_ protocol.VersionNumber) protocol.ByteCount { length := protocol.ByteCount(1 + 2 + 1) // 1 TypeByte, 2 ACK delay time, 1 Num Timestamp length += protocol.ByteCount(protocol.GetPacketNumberLength(f.LargestAcked)) @@ -320,7 +320,7 @@ func (f *AckFrame) minLengthLegacy(_ protocol.VersionNumber) (protocol.ByteCount length += missingSequenceNumberDeltaLen } // we don't write - return length, nil + return length } // numWritableNackRanges calculates the number of ACK blocks that are about to be written diff --git a/internal/wire/blocked_frame.go b/internal/wire/blocked_frame.go index cc6a01651..72c8a0561 100644 --- a/internal/wire/blocked_frame.go +++ b/internal/wire/blocked_frame.go @@ -27,9 +27,9 @@ func (f *BlockedFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) er } // MinLength of a written frame -func (f *BlockedFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *BlockedFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if !version.UsesIETFFrameFormat() { // writing this frame would result in a legacy BLOCKED being written, which is longer - return 1 + 4, nil + return 1 + 4 } - return 1, nil + return 1 } diff --git a/internal/wire/connection_close_frame.go b/internal/wire/connection_close_frame.go index 2cad86503..ccc3a71d4 100644 --- a/internal/wire/connection_close_frame.go +++ b/internal/wire/connection_close_frame.go @@ -68,11 +68,11 @@ func ParseConnectionCloseFrame(r *bytes.Reader, version protocol.VersionNumber) } // MinLength of a written frame -func (f *ConnectionCloseFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *ConnectionCloseFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if version.UsesIETFFrameFormat() { - return 1 + 2 + utils.VarIntLen(uint64(len(f.ReasonPhrase))) + protocol.ByteCount(len(f.ReasonPhrase)), nil + return 1 + 2 + utils.VarIntLen(uint64(len(f.ReasonPhrase))) + protocol.ByteCount(len(f.ReasonPhrase)) } - return 1 + 4 + 2 + protocol.ByteCount(len(f.ReasonPhrase)), nil + return 1 + 4 + 2 + protocol.ByteCount(len(f.ReasonPhrase)) } // Write writes an CONNECTION_CLOSE frame. diff --git a/internal/wire/frame.go b/internal/wire/frame.go index f31f5bf28..d9f0cea3f 100644 --- a/internal/wire/frame.go +++ b/internal/wire/frame.go @@ -9,5 +9,5 @@ import ( // A Frame in QUIC type Frame interface { Write(b *bytes.Buffer, version protocol.VersionNumber) error - MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) + MinLength(version protocol.VersionNumber) protocol.ByteCount } diff --git a/internal/wire/goaway_frame.go b/internal/wire/goaway_frame.go index 44a613c87..fa5585a75 100644 --- a/internal/wire/goaway_frame.go +++ b/internal/wire/goaway_frame.go @@ -63,6 +63,6 @@ func (f *GoawayFrame) Write(b *bytes.Buffer, _ protocol.VersionNumber) error { } // MinLength of a written frame -func (f *GoawayFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { - return protocol.ByteCount(1 + 4 + 4 + 2 + len(f.ReasonPhrase)), nil +func (f *GoawayFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { + return protocol.ByteCount(1 + 4 + 4 + 2 + len(f.ReasonPhrase)) } diff --git a/internal/wire/max_data_frame.go b/internal/wire/max_data_frame.go index 19585bc66..945d11abb 100644 --- a/internal/wire/max_data_frame.go +++ b/internal/wire/max_data_frame.go @@ -43,9 +43,9 @@ func (f *MaxDataFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) er } // MinLength of a written frame -func (f *MaxDataFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *MaxDataFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if !version.UsesIETFFrameFormat() { // writing this frame would result in a gQUIC WINDOW_UPDATE being written, which is longer - return 1 + 4 + 8, nil + return 1 + 4 + 8 } - return 1 + utils.VarIntLen(uint64(f.ByteOffset)), nil + return 1 + utils.VarIntLen(uint64(f.ByteOffset)) } diff --git a/internal/wire/max_stream_data_frame.go b/internal/wire/max_stream_data_frame.go index 810dc92a3..548887663 100644 --- a/internal/wire/max_stream_data_frame.go +++ b/internal/wire/max_stream_data_frame.go @@ -51,10 +51,10 @@ func (f *MaxStreamDataFrame) Write(b *bytes.Buffer, version protocol.VersionNumb } // MinLength of a written frame -func (f *MaxStreamDataFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *MaxStreamDataFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { // writing this frame would result in a gQUIC WINDOW_UPDATE being written, which has a different length if !version.UsesIETFFrameFormat() { - return 1 + 4 + 8, nil + return 1 + 4 + 8 } - return 1 + utils.VarIntLen(uint64(f.StreamID)) + utils.VarIntLen(uint64(f.ByteOffset)), nil + return 1 + utils.VarIntLen(uint64(f.StreamID)) + utils.VarIntLen(uint64(f.ByteOffset)) } diff --git a/internal/wire/ping_frame.go b/internal/wire/ping_frame.go index 2a09c33a7..c7fdc40a7 100644 --- a/internal/wire/ping_frame.go +++ b/internal/wire/ping_frame.go @@ -28,6 +28,6 @@ func (f *PingFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) error } // MinLength of a written frame -func (f *PingFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { - return 1, nil +func (f *PingFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { + return 1 } diff --git a/internal/wire/rst_stream_frame.go b/internal/wire/rst_stream_frame.go index 05a4cad5b..d659a4a0f 100644 --- a/internal/wire/rst_stream_frame.go +++ b/internal/wire/rst_stream_frame.go @@ -80,9 +80,9 @@ func (f *RstStreamFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) } // MinLength of a written frame -func (f *RstStreamFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *RstStreamFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if version.UsesIETFFrameFormat() { - return 1 + utils.VarIntLen(uint64(f.StreamID)) + 2 + utils.VarIntLen(uint64(f.ByteOffset)), nil + return 1 + utils.VarIntLen(uint64(f.StreamID)) + 2 + utils.VarIntLen(uint64(f.ByteOffset)) } - return 1 + 4 + 8 + 4, nil + return 1 + 4 + 8 + 4 } diff --git a/internal/wire/stop_waiting_frame.go b/internal/wire/stop_waiting_frame.go index 1f4668856..4f7a1c8bc 100644 --- a/internal/wire/stop_waiting_frame.go +++ b/internal/wire/stop_waiting_frame.go @@ -49,14 +49,8 @@ func (f *StopWaitingFrame) Write(b *bytes.Buffer, _ protocol.VersionNumber) erro } // MinLength of a written frame -func (f *StopWaitingFrame) MinLength(_ protocol.VersionNumber) (protocol.ByteCount, error) { - minLength := protocol.ByteCount(1) // typeByte - - if f.PacketNumberLen == protocol.PacketNumberLenInvalid { - return 0, errPacketNumberLenNotSet - } - minLength += protocol.ByteCount(f.PacketNumberLen) - return minLength, nil +func (f *StopWaitingFrame) MinLength(_ protocol.VersionNumber) protocol.ByteCount { + return 1 + protocol.ByteCount(f.PacketNumberLen) } // ParseStopWaitingFrame parses a StopWaiting frame diff --git a/internal/wire/stop_waiting_frame_test.go b/internal/wire/stop_waiting_frame_test.go index ec22c306d..fb0f61c37 100644 --- a/internal/wire/stop_waiting_frame_test.go +++ b/internal/wire/stop_waiting_frame_test.go @@ -176,14 +176,6 @@ var _ = Describe("StopWaitingFrame", func() { Expect(frame.MinLength(protocol.VersionWhatever)).To(Equal(protocol.ByteCount(length + 1))) } }) - - It("errors when packetNumberLen is not set", func() { - frame := &StopWaitingFrame{ - LeastUnacked: 10, - } - _, err := frame.MinLength(0) - Expect(err).To(MatchError(errPacketNumberLenNotSet)) - }) }) Context("self consistency", func() { diff --git a/internal/wire/stream_blocked_frame.go b/internal/wire/stream_blocked_frame.go index 510de50d7..b2ecd5845 100644 --- a/internal/wire/stream_blocked_frame.go +++ b/internal/wire/stream_blocked_frame.go @@ -35,9 +35,9 @@ func (f *StreamBlockedFrame) Write(b *bytes.Buffer, version protocol.VersionNumb } // MinLength of a written frame -func (f *StreamBlockedFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *StreamBlockedFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if !version.UsesIETFFrameFormat() { - return 1 + 4, nil + return 1 + 4 } - return 1 + utils.VarIntLen(uint64(f.StreamID)), nil + return 1 + utils.VarIntLen(uint64(f.StreamID)) } diff --git a/internal/wire/stream_frame.go b/internal/wire/stream_frame.go index fc38acd07..6be006596 100644 --- a/internal/wire/stream_frame.go +++ b/internal/wire/stream_frame.go @@ -117,7 +117,7 @@ func (f *StreamFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) err // MinLength returns the length of the header of a StreamFrame // the total length of the frame is frame.MinLength() + frame.DataLen() -func (f *StreamFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *StreamFrame) MinLength(version protocol.VersionNumber) protocol.ByteCount { if !version.UsesIETFFrameFormat() { return f.minLengthLegacy(version) } @@ -128,5 +128,5 @@ func (f *StreamFrame) MinLength(version protocol.VersionNumber) (protocol.ByteCo if f.DataLenPresent { length += utils.VarIntLen(uint64(f.DataLen())) } - return length, nil + return length } diff --git a/internal/wire/stream_frame_legacy.go b/internal/wire/stream_frame_legacy.go index e3687cb97..c44c2556e 100644 --- a/internal/wire/stream_frame_legacy.go +++ b/internal/wire/stream_frame_legacy.go @@ -183,12 +183,12 @@ func (f *StreamFrame) getOffsetLength() protocol.ByteCount { return 8 } -func (f *StreamFrame) minLengthLegacy(_ protocol.VersionNumber) (protocol.ByteCount, error) { +func (f *StreamFrame) minLengthLegacy(_ protocol.VersionNumber) protocol.ByteCount { length := protocol.ByteCount(1) + protocol.ByteCount(f.calculateStreamIDLength()) + f.getOffsetLength() if f.DataLenPresent { length += 2 } - return length, nil + return length } // DataLen gives the length of data in bytes diff --git a/internal/wire/stream_frame_legacy_test.go b/internal/wire/stream_frame_legacy_test.go index b7b8d2569..b179a0dd9 100644 --- a/internal/wire/stream_frame_legacy_test.go +++ b/internal/wire/stream_frame_legacy_test.go @@ -210,7 +210,7 @@ var _ = Describe("STREAM frame (for gQUIC)", func() { } err := f.Write(b, versionBigEndian) Expect(err).ToNot(HaveOccurred()) - minLength, _ := f.MinLength(0) + minLength := f.MinLength(0) Expect(b.Bytes()[0] & 0x20).To(Equal(uint8(0x20))) Expect(b.Bytes()[minLength-2 : minLength]).To(Equal([]byte{0x13, 0x37})) }) @@ -229,9 +229,9 @@ var _ = Describe("STREAM frame (for gQUIC)", func() { Expect(err).ToNot(HaveOccurred()) Expect(b.Bytes()[0] & 0x20).To(Equal(uint8(0))) Expect(b.Bytes()[1 : b.Len()-dataLen]).ToNot(ContainSubstring(string([]byte{0x37, 0x13}))) - minLength, _ := f.MinLength(versionBigEndian) + minLength := f.MinLength(versionBigEndian) f.DataLenPresent = true - minLengthWithoutDataLen, _ := f.MinLength(versionBigEndian) + minLengthWithoutDataLen := f.MinLength(versionBigEndian) Expect(minLength).To(Equal(minLengthWithoutDataLen - 2)) }) @@ -242,7 +242,7 @@ var _ = Describe("STREAM frame (for gQUIC)", func() { DataLenPresent: false, Offset: 0xdeadbeef, } - minLengthWithoutDataLen, _ := f.MinLength(versionBigEndian) + minLengthWithoutDataLen := f.MinLength(versionBigEndian) f.DataLenPresent = true Expect(f.MinLength(versionBigEndian)).To(Equal(minLengthWithoutDataLen + 2)) }) diff --git a/packet_packer.go b/packet_packer.go index aabddee1a..badbc81f3 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -199,27 +199,17 @@ func (p *packetPacker) composeNextPacket( // STOP_WAITING and ACK will always fit if p.stopWaiting != nil { payloadFrames = append(payloadFrames, p.stopWaiting) - l, err := p.stopWaiting.MinLength(p.version) - if err != nil { - return nil, err - } - payloadLength += l + payloadLength += p.stopWaiting.MinLength(p.version) } if p.ackFrame != nil { payloadFrames = append(payloadFrames, p.ackFrame) - l, err := p.ackFrame.MinLength(p.version) - if err != nil { - return nil, err - } + l := p.ackFrame.MinLength(p.version) payloadLength += l } for len(p.controlFrames) > 0 { frame := p.controlFrames[len(p.controlFrames)-1] - minLength, err := frame.MinLength(p.version) - if err != nil { - return nil, err - } + minLength := frame.MinLength(p.version) if payloadLength+minLength > maxFrameSize { break } diff --git a/packet_packer_test.go b/packet_packer_test.go index 50cedd90e..0771f038b 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -329,8 +329,7 @@ var _ = Describe("Packet packer", func() { It("packs a lot of control frames into 2 packets if they don't fit into one", func() { blockedFrame := &wire.BlockedFrame{} - minLength, _ := blockedFrame.MinLength(packer.version) - maxFramesPerPacket := int(maxFrameSize) / int(minLength) + maxFramesPerPacket := int(maxFrameSize) / int(blockedFrame.MinLength(packer.version)) var controlFrames []wire.Frame for i := 0; i < maxFramesPerPacket+10; i++ { controlFrames = append(controlFrames, blockedFrame) @@ -369,8 +368,7 @@ var _ = Describe("Packet packer", func() { StreamID: 5, DataLenPresent: false, } - minLength, _ := f.MinLength(packer.version) - maxStreamFrameDataLen := maxFrameSize - minLength + maxStreamFrameDataLen := maxFrameSize - f.MinLength(packer.version) f.Data = bytes.Repeat([]byte{'f'}, int(maxStreamFrameDataLen)) streamFramer.AddFrameForRetransmission(f) payloadFrames, err := packer.composeNextPacket(maxFrameSize, true) @@ -390,10 +388,9 @@ var _ = Describe("Packet packer", func() { StreamID: 5, DataLenPresent: true, } - minLength, _ := f.MinLength(packer.version) // for IETF draft style STREAM frames, we don't know the size of the DataLen, because it is a variable length integer // in the general case, we therefore use a STREAM frame that is 1 byte smaller than the maximum size - maxStreamFrameDataLen := maxFrameSize - minLength - 1 + maxStreamFrameDataLen := maxFrameSize - f.MinLength(packer.version) - 1 f.Data = bytes.Repeat([]byte{'f'}, int(maxStreamFrameDataLen)) streamFramer.AddFrameForRetransmission(f) payloadFrames, err := packer.composeNextPacket(maxFrameSize, true) @@ -467,8 +464,7 @@ var _ = Describe("Packet packer", func() { StreamID: 7, Offset: 1, } - minLength, _ := f.MinLength(packer.version) - maxStreamFrameDataLen := maxFrameSize - minLength + maxStreamFrameDataLen := maxFrameSize - f.MinLength(packer.version) f.Data = bytes.Repeat([]byte{'f'}, int(maxStreamFrameDataLen)+200) streamFramer.AddFrameForRetransmission(f) payloadFrames, err := packer.composeNextPacket(maxFrameSize, true) @@ -526,8 +522,7 @@ var _ = Describe("Packet packer", func() { StreamID: 5, Offset: 1, } - minLength, _ := f.MinLength(packer.version) - f.Data = bytes.Repeat([]byte{'f'}, int(maxFrameSize-minLength+1)) // + 1 since MinceLength is 1 bigger than the actual StreamFrame header + f.Data = bytes.Repeat([]byte{'f'}, int(maxFrameSize-f.MinLength(packer.version)+1)) // + 1 since MinceLength is 1 bigger than the actual StreamFrame header streamFramer.AddFrameForRetransmission(f) p, err := packer.PackPacket() Expect(err).ToNot(HaveOccurred()) @@ -540,8 +535,7 @@ var _ = Describe("Packet packer", func() { StreamID: 5, Offset: 1, } - minLength, _ := f.MinLength(packer.version) - f.Data = bytes.Repeat([]byte{'f'}, int(maxFrameSize-minLength+2)) // + 2 since MinceLength is 1 bigger than the actual StreamFrame header + f.Data = bytes.Repeat([]byte{'f'}, int(maxFrameSize-f.MinLength(packer.version)+2)) // + 2 since MinceLength is 1 bigger than the actual StreamFrame header streamFramer.AddFrameForRetransmission(f) payloadFrames, err := packer.composeNextPacket(maxFrameSize, true) diff --git a/stream_framer.go b/stream_framer.go index e275fccd6..a0ab8ae6d 100644 --- a/stream_framer.go +++ b/stream_framer.go @@ -66,8 +66,7 @@ func (f *streamFramer) PopCryptoStreamFrame(maxLen protocol.ByteCount) *wire.Str StreamID: f.cryptoStream.StreamID(), Offset: f.cryptoStream.GetWriteOffset(), } - frameHeaderBytes, _ := frame.MinLength(f.version) // can never error - frame.Data, frame.FinBit = f.cryptoStream.GetDataForWriting(maxLen - frameHeaderBytes) + frame.Data, frame.FinBit = f.cryptoStream.GetDataForWriting(maxLen - frame.MinLength(f.version)) return frame } @@ -76,11 +75,10 @@ func (f *streamFramer) maybePopFramesForRetransmission(maxLen protocol.ByteCount frame := f.retransmissionQueue[0] frame.DataLenPresent = true - frameHeaderLen, _ := frame.MinLength(f.version) // can never error + frameHeaderLen := frame.MinLength(f.version) if currentLen+frameHeaderLen >= maxLen { break } - currentLen += frameHeaderLen splitFrame := maybeSplitOffFrame(frame, maxLen-currentLen) @@ -109,7 +107,7 @@ func (f *streamFramer) maybePopNormalFrames(maxBytes protocol.ByteCount) (res [] frame.StreamID = s.StreamID() frame.Offset = s.GetWriteOffset() // not perfect, but thread-safe since writeOffset is only written when getting data - frameHeaderBytes, _ := frame.MinLength(f.version) // can never error + frameHeaderBytes := frame.MinLength(f.version) if currentLen+frameHeaderBytes > maxBytes { return false, nil // theoretically, we could find another stream that fits, but this is quite unlikely, so we stop here } diff --git a/stream_framer_test.go b/stream_framer_test.go index 0dabce537..65314b80f 100644 --- a/stream_framer_test.go +++ b/stream_framer_test.go @@ -218,7 +218,7 @@ var _ = Describe("Stream Framer", func() { origlen := retransmittedFrame2.DataLen() fs := framer.PopStreamFrames(6) Expect(fs).To(HaveLen(1)) - minLength, _ := fs[0].MinLength(framer.version) + minLength := fs[0].MinLength(framer.version) Expect(minLength + fs[0].DataLen()).To(Equal(protocol.ByteCount(6))) Expect(framer.retransmissionQueue[0].Data).To(HaveLen(int(origlen - fs[0].DataLen()))) Expect(framer.retransmissionQueue[0].Offset).To(Equal(fs[0].DataLen()))