Bug 2104 – Sequence Number passed to QosUtilsMapSeqControlToUniqueInteger instead of Sequence Control

Bug 2104 - Sequence Number passed to QosUtilsMapSeqControlToUniqueInteger instead of Sequence Control


Summary:	Sequence Number passed to QosUtilsMapSeqControlToUniqueInteger instead of Seq...

Status:	RESOLVED FIXED

Product:	ns-3
Classification:	Unclassified
Component:	wifi
Version:	ns-3-dev
Hardware:	All All

Importance:	P3 major
Assigned To:	sebastien.deronne

URL:
Keywords:

Depends on:
Blocks:
	Show dependency tree / graph

Reported:	2015-04-22 06:45 UTC by Steven Sultana
Modified:	2015-09-01 16:48 UTC (History)
CC List:	3 users (show)

See Also:

Attachments
Patch to mac-low.* (2.35 KB, text/plain) 2015-04-22 06:45 UTC, Steven Sultana	Details
Updated to include fixes for all instances RxCompleteBufferedPacketsWithSmallerSequence (4.13 KB, patch) 2015-05-11 11:12 UTC, Steven Sultana	Details \| Diff
revised version of the patch (9.06 KB, patch) 2015-09-01 15:06 UTC, sebastien.deronne	Details \| Diff
Show Obsolete (2) Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Steven Sultana 2015-04-22 06:45:37 UTC

Created attachment 2026 [details]
Patch to mac-low.*

The QosUtilsMapSeqControlToUniqueInteger function expects a Sequence Control as the first parameter, but a Sequence Number is passed in MacLow::RxCompleteBufferedPacketsWithSmallerSequence.

Patch attached.

Comment 1 Steven Sultana 2015-05-11 11:12:44 UTC

Created attachment 2038 [details]
Updated to include fixes for all instances RxCompleteBufferedPacketsWithSmallerSequence

Comment 2 sebastien.deronne 2015-08-29 09:01:41 UTC

I agree with the fix, and furthermore it appears to fix an important issue encountered when using TCP with A-MPDU. Unless someone does not agree with the proposed patch, I suggest to deliver it before the upcoming release.

Comment 3 Tommaso Pecorella 2015-08-30 09:25:49 UTC

I agree on pushing it,but it needs one important sub-fix: GetSequenceControl seems to be bugged (!)

In many places in the code, the Starting Sequence Control is used, but it is simply calculated on the fly, e.g.:
      uint16_t startingSeqCtrl = ((*it).second.first.GetStartingSequence () << 4) & 0xfff0;
      uint16_t guard = startingSeqCtrl;


Now, GetSequenceControl is similar but not identical:
  uint16_t seqControl = (m_startingSeq << 4) | 0xfff0;

OR instead of AND... basically GetStartingSequenceControl will always return 0xfff0.
Note that GetStartingSequenceControl is never used in the code (!!) and it should be, in order to avoid small and scattered bugs (e.g., it should have been used in the "uint16_t startingSeqCtrl" setup.

Beside this, I agree that this should be pushed before 3.24, but please triple check the patch behaviour after fixing GetStartingSequenceControl.

Comment 4 sebastien.deronne 2015-08-30 10:07:30 UTC

Thanks Tommaso.
I will work on a revised patch then.

Comment 5 sebastien.deronne 2015-09-01 15:06:01 UTC

Created attachment 2136 [details]
revised version of the patch

I made a revised patch for this issue, with regards to Tommaso's comments.
The fix is quite straightforward, it should be delivered asap so that it can be included in the upcoming release.

Comment 6 Tom Henderson 2015-09-01 15:24:34 UTC

(In reply to sebastien.deronne from comment #5)
> Created attachment 2136 [details]
> revised version of the patch
> 
> I made a revised patch for this issue, with regards to Tommaso's comments.
> The fix is quite straightforward, it should be delivered asap so that it can
> be included in the upcoming release.

Please commit it when you are ready.

Comment 7 sebastien.deronne 2015-09-01 15:26:26 UTC

Tom, for me it is ready, and it improves TCP over A-MPDU.

Comment 8 sebastien.deronne 2015-09-01 16:48:03 UTC

Committed in changeset 11622:d0da0aa55bda