From 250fbb3c48feda1425219b61043da9bb2edbb99f Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Tue, 13 Apr 2021 11:11:04 +0200 Subject: [PATCH] dcsctp: Make Sequence Number API more consistent * `AddTo` and `Difference` are made into static methods, as one may have believed that these modified the current object previously. The `Increment` method is kept, as it's obvious that it modifies the current object as it doesn't have a return value, and `next_value` is kept, as its naming (lower-case, snake) indicates that it's a simple accessor. * Difference will return the absolute difference. This is actually the only reasonable choice, as the return value was unsigned and any negative value would just wrap. Bug: webrtc:12614 Change-Id: If14a71636e67fc612d12759dc80a9c2518c85281 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215069 Reviewed-by: Tommi Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/master@{#33714} --- net/dcsctp/common/sequence_numbers.h | 21 ++++++---- net/dcsctp/common/sequence_numbers_test.cc | 39 +++++++++++++------ net/dcsctp/rx/data_tracker.cc | 7 ++-- .../rx/traditional_reassembly_streams.cc | 3 +- 4 files changed, 47 insertions(+), 23 deletions(-) diff --git a/net/dcsctp/common/sequence_numbers.h b/net/dcsctp/common/sequence_numbers.h index 4f318c3861..52b638b54a 100644 --- a/net/dcsctp/common/sequence_numbers.h +++ b/net/dcsctp/common/sequence_numbers.h @@ -121,19 +121,26 @@ class UnwrappedSequenceNumber { // Increments the value. void Increment() { ++value_; } + + // Returns the next value relative to this sequence number. UnwrappedSequenceNumber next_value() const { return UnwrappedSequenceNumber(value_ + 1); } - // Adds a delta to the current value. - UnwrappedSequenceNumber AddTo(int delta) const { - return UnwrappedSequenceNumber(value_ + delta); + // Returns a new sequence number based on `value`, and adding `delta` (which + // may be negative). + static UnwrappedSequenceNumber AddTo( + UnwrappedSequenceNumber value, + int delta) { + return UnwrappedSequenceNumber(value.value_ + delta); } - // Compares the difference between two sequence numbers. - typename WrappedType::UnderlyingType Difference( - UnwrappedSequenceNumber other) const { - return value_ - other.value_; + // Returns the absolute difference between `lhs` and `rhs`. + static typename WrappedType::UnderlyingType Difference( + UnwrappedSequenceNumber lhs, + UnwrappedSequenceNumber rhs) { + return (lhs.value_ > rhs.value_) ? (lhs.value_ - rhs.value_) + : (rhs.value_ - lhs.value_); } private: diff --git a/net/dcsctp/common/sequence_numbers_test.cc b/net/dcsctp/common/sequence_numbers_test.cc index d1d1959491..f5fa788876 100644 --- a/net/dcsctp/common/sequence_numbers_test.cc +++ b/net/dcsctp/common/sequence_numbers_test.cc @@ -32,9 +32,9 @@ TEST(SequenceNumbersTest, SimpleUnwrapping) { EXPECT_LT(s1, s3); EXPECT_LT(s2, s3); - EXPECT_EQ(s1.Difference(s0), 1); - EXPECT_EQ(s2.Difference(s0), 2); - EXPECT_EQ(s3.Difference(s0), 3); + EXPECT_EQ(TestSequence::Difference(s1, s0), 1); + EXPECT_EQ(TestSequence::Difference(s2, s0), 2); + EXPECT_EQ(TestSequence::Difference(s3, s0), 3); EXPECT_GT(s1, s0); EXPECT_GT(s2, s0); @@ -50,7 +50,7 @@ TEST(SequenceNumbersTest, SimpleUnwrapping) { s2.Increment(); EXPECT_EQ(s2, s3); - EXPECT_EQ(s0.AddTo(2), s3); + EXPECT_EQ(TestSequence::AddTo(s0, 2), s3); } TEST(SequenceNumbersTest, MidValueUnwrapping) { @@ -68,9 +68,9 @@ TEST(SequenceNumbersTest, MidValueUnwrapping) { EXPECT_LT(s1, s3); EXPECT_LT(s2, s3); - EXPECT_EQ(s1.Difference(s0), 1); - EXPECT_EQ(s2.Difference(s0), 2); - EXPECT_EQ(s3.Difference(s0), 3); + EXPECT_EQ(TestSequence::Difference(s1, s0), 1); + EXPECT_EQ(TestSequence::Difference(s2, s0), 2); + EXPECT_EQ(TestSequence::Difference(s3, s0), 3); EXPECT_GT(s1, s0); EXPECT_GT(s2, s0); @@ -86,7 +86,7 @@ TEST(SequenceNumbersTest, MidValueUnwrapping) { s2.Increment(); EXPECT_EQ(s2, s3); - EXPECT_EQ(s0.AddTo(2), s3); + EXPECT_EQ(TestSequence::AddTo(s0, 2), s3); } TEST(SequenceNumbersTest, WrappedUnwrapping) { @@ -104,9 +104,9 @@ TEST(SequenceNumbersTest, WrappedUnwrapping) { EXPECT_LT(s1, s3); EXPECT_LT(s2, s3); - EXPECT_EQ(s1.Difference(s0), 1); - EXPECT_EQ(s2.Difference(s0), 2); - EXPECT_EQ(s3.Difference(s0), 3); + EXPECT_EQ(TestSequence::Difference(s1, s0), 1); + EXPECT_EQ(TestSequence::Difference(s2, s0), 2); + EXPECT_EQ(TestSequence::Difference(s3, s0), 3); EXPECT_GT(s1, s0); EXPECT_GT(s2, s0); @@ -122,7 +122,7 @@ TEST(SequenceNumbersTest, WrappedUnwrapping) { s2.Increment(); EXPECT_EQ(s2, s3); - EXPECT_EQ(s0.AddTo(2), s3); + EXPECT_EQ(TestSequence::AddTo(s0, 2), s3); } TEST(SequenceNumbersTest, WrapAroundAFewTimes) { @@ -183,5 +183,20 @@ TEST(SequenceNumbersTest, UnwrappingSmallerNumberIsAlwaysSmaller) { } } +TEST(SequenceNumbersTest, DifferenceIsAbsolute) { + TestSequence::Unwrapper unwrapper; + + TestSequence this_value = unwrapper.Unwrap(Wrapped(10)); + TestSequence other_value = TestSequence::AddTo(this_value, 100); + + EXPECT_EQ(TestSequence::Difference(this_value, other_value), 100); + EXPECT_EQ(TestSequence::Difference(other_value, this_value), 100); + + TestSequence minus_value = TestSequence::AddTo(this_value, -100); + + EXPECT_EQ(TestSequence::Difference(this_value, minus_value), 100); + EXPECT_EQ(TestSequence::Difference(minus_value, this_value), 100); +} + } // namespace } // namespace dcsctp diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index 9e5cbe2980..b95cb44230 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -179,9 +179,10 @@ std::vector DataTracker::CreateGapAckBlocks() const { auto flush = [&]() { if (first_tsn_in_block.has_value()) { - int start_diff = - first_tsn_in_block->Difference(last_cumulative_acked_tsn_); - int end_diff = last_tsn_in_block->Difference(last_cumulative_acked_tsn_); + int start_diff = UnwrappedTSN::Difference(*first_tsn_in_block, + last_cumulative_acked_tsn_); + int end_diff = UnwrappedTSN::Difference(*last_tsn_in_block, + last_cumulative_acked_tsn_); gap_ack_blocks.emplace_back(static_cast(start_diff), static_cast(end_diff)); first_tsn_in_block = absl::nullopt; diff --git a/net/dcsctp/rx/traditional_reassembly_streams.cc b/net/dcsctp/rx/traditional_reassembly_streams.cc index caa97d2f66..7cec1150d5 100644 --- a/net/dcsctp/rx/traditional_reassembly_streams.cc +++ b/net/dcsctp/rx/traditional_reassembly_streams.cc @@ -172,7 +172,8 @@ size_t TraditionalReassemblyStreams::OrderedStream::TryToAssembleMessage() { return 0; } - uint32_t tsn_diff = chunks.rbegin()->first.Difference(chunks.begin()->first); + uint32_t tsn_diff = + UnwrappedTSN::Difference(chunks.rbegin()->first, chunks.begin()->first); if (tsn_diff != chunks.size() - 1) { return 0; }