[ns] Bugfixes/enhancements for scoreboard.cc

[Ethan Blanton <eblanton@cs.ohiou.edu>]

Hi all,
I believe I have found and fixed two bugs in the scoreboard code for SACK.
Attached to this message is a diff against scoreboard.cc containing these
changes.

The first involves creation of a scoreboard; the existing code creates a
scoreboard any time a packet is processed by UpdateScoreboard(); I believe
this should only happen when the processed packet contains a SACK block.
The fix is simply to check that tcph->sa_length() is nonzero before creating
a scoreboard.  A SACK-less packet falls straight through this check,
preventing from creating an invalid scoreboard (which is truncated upon
receipt of a packet containing SACK information, anyway).

The second fix also involves SACK-less packets processed by
UpdateScoreboard().  The check for advancing the left edge of the stored
SACK information is only processed for a packet containing SACK information;
this prevents a cumulative acknowledgement that covers SACKed packets from
properly updating the scoreboard.  The attached fix moves this check outside
of the per-SACK-block portion of code and into the globally-executed
portion.  (This has the added side effect of not trying to update a
packet-dependent characteristic on a per-SACK-block basis, as it can only
occur once per packet)

Please let me know if you have thoughts on this code, it seems to work well
for me.
Ethan
--- scoreboard.cc	Wed Jul  5 13:54:33 2000
+++ ns-2/scoreboard.cc	Wed Jul  5 12:58:28 2000
@@ -64,7 +64,7 @@
 	int retran_decr = 0;
 
 	//  If there is no scoreboard, create one.
-	if (length_ == 0) {
+	if (length_ == 0 && tcph->sa_length()) {
 		i = last_ack+1;
 		SBNI.seq_no_ = i;
 		SBNI.ack_flag_ = 0;
@@ -78,7 +78,29 @@
 			exit(1);
 		}
 	}	
-
+	
+	//  Advance the left edge of the block.
+	if (SBN[first_].seq_no_ <= last_ack) {
+		for (i=SBN[(first_)%SBSIZE].seq_no_; i<=last_ack; i++) {
+			//  Advance the ACK
+			if (SBNI.seq_no_ <= last_ack) {
+				ASSERT(first_ == i%SBSIZE);
+				first_ = (first_+1)%SBSIZE; 
+				length_--;
+				ASSERT1(length_ >= 0);
+				SBNI.ack_flag_ = 1;
+				SBNI.sack_flag_ = 1;
+				if (SBNI.retran_) {
+					SBNI.retran_ = 0;
+					SBNI.snd_nxt_ = 0;
+					retran_decr++;
+				}
+				if (length_==0) 
+					break;
+			}
+		}
+	}
+	
 	for (sack_index=0; sack_index < tcph->sa_length(); sack_index++) {
 		sack_left = tcph->sa_left(sack_index);
 		sack_right = tcph->sa_right(sack_index);
@@ -99,28 +124,6 @@
 				}
 			}
 		}
-
-		//  Advance the left edge of the block.
-		if (SBN[first_].seq_no_ <= last_ack) {
-			for (i=SBN[(first_)%SBSIZE].seq_no_; i<=last_ack; i++) {
-				//  Advance the ACK
-				if (SBNI.seq_no_ <= last_ack) {
-					ASSERT(first_ == i%SBSIZE);
-					first_ = (first_+1)%SBSIZE; 
-					length_--;
-					ASSERT1(length_ >= 0);
-					SBNI.ack_flag_ = 1;
-					SBNI.sack_flag_ = 1;
-					if (SBNI.retran_) {
-						SBNI.retran_ = 0;
-						SBNI.snd_nxt_ = 0;
-						retran_decr++;
-					}
-					if (length_==0) 
-					  break;
-				}
-			}
-		}
 		
 		for (i=SBN[(first_)%SBSIZE].seq_no_; i<sack_right; i++) {
 			//  Check to see if this segment is now covered by the sack block