Bug 2487 - ByteTag scope off-by-one error
ByteTag scope off-by-one error
Status: PATCH PENDING
Product: ns-3
Classification: Unclassified
Component: network
unspecified
All All
: P5 normal
Assigned To: ns-bugs
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-29 14:36 EDT by Tom Henderson
Modified: 2018-07-26 13:54 EDT (History)
2 users (show)

See Also:


Attachments
test case (3.09 KB, text/x-c++src)
2016-08-29 14:36 EDT, Tom Henderson
Details
revised test case using PrintByteTags () (3.58 KB, text/plain)
2018-07-26 13:11 EDT, Tom Henderson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2016-08-29 14:36:48 EDT
Created attachment 2561 [details]
test case

Byte tags apply to all of the bytes of the packet that they were added to.  In the case that packets are merged, one can use a ByteTagIterator to learn the scope of each ByteTag.  

The class ByteTagList::Iterator::Item has these fields:

      int32_t start;          //!< offset to the start of the tag from the virtual byte buffer
      int32_t end;            //!< offset to the end of the tag from the virtual byte buffer


When two 500 byte packets, each with a tag, are merged with Packet::AddAtEnd(), and the byte tags examined, the start and end values evaluate as follows (from attached test program):

Byte tag found from byte 0 to 500 with value 2
Byte tag found from byte 500 to 1000 with value 1

Index values 0-1000 span 1001 bytes; byte 500 is seemingly covered twice.

The doxygen states:

    /**
     * \returns the index of the first byte tagged by this tag.
     *
     * \brief The index is an offset from the start of the packet.
     */
    uint32_t GetStart (void) const
    /**
     * \returns the index of the last byte tagged by this tag.
     *
     * \brief The index is an offset from the start of the packet.
     */
    uint32_t GetEnd (void) const;


According to the above, it seems to me that the GetEnd () should instead return 499 if GetStart () returns 0.
Comment 1 Tommaso Pecorella 2016-08-29 19:38:57 EDT
I think that the original intent was to mimic the iterator::end() functionality, i.e., the "end" is the past-the-end element in the sequence.

Of course this might be a documentation issue, a problem in how we use the ByteTags or both.
Comment 2 Alexander Krotov 2018-07-26 09:09:48 EDT
Doxygen documentation is correct. If a packet has size of 5 bytes, byte tag with start 0 and end 5 covers the packet. *Offset* of the start is 0, and *offset* of the end is 5 bytes (40 bits).

See also the implementation of Packet::AddByteTag. It sets start to 0 and end to GetSize ().

Shouldn't we close this "bug" with WONTFIX?
Comment 3 Tom Henderson 2018-07-26 11:46:19 EDT
(In reply to Alexander Krotov from comment #2)
> Doxygen documentation is correct. If a packet has size of 5 bytes, byte tag
> with start 0 and end 5 covers the packet. *Offset* of the start is 0, and
> *offset* of the end is 5 bytes (40 bits).
> 
> See also the implementation of Packet::AddByteTag. It sets start to 0 and
> end to GetSize ().
> 
> Shouldn't we close this "bug" with WONTFIX?

The implementation is correct, but I am questioning the documentation equating 'index' and 'offset'.

For a five byte packet, it can be stored in a vector of size 5, with byte indices 0, 1, 2, 3, 4.  The index of the last byte is 4, but the offset is 5.

My proposal: 

diff -r 2c35236a1cdf src/network/model/packet.h
--- a/src/network/model/packet.h        Sat Jul 21 16:31:05 2018 -0700
+++ b/src/network/model/packet.h        Thu Jul 26 08:45:19 2018 -0700
@@ -72,9 +72,14 @@
      */
     uint32_t GetStart (void) const;
     /**
-     * \returns the index of the last byte tagged by this tag.
+     * \returns the index of the first byte beyond the last byte tagged by this tag.
      *
-     * \brief The index is an offset from the start of the packet.
+     * The index value returned is analogous to the C++ std::end() method 
+     * that points past-the-end of the last element in the sequence.  As
+     * an example, for a 1000 byte packet for which all bytes are tagged,
+     * GetStart () would return 0, and GetEnd () would return 1000. 
+     * The quantity (GetEnd () - GetStart ()) is the number of consecutive  
+     * bytes tagged.
      */
     uint32_t GetEnd (void) const;
     /**
Comment 4 Alexander Krotov 2018-07-26 12:13:01 EDT
Maybe just change "index" to "offset"?
Like this:

\returns the offset to the end of the tag from the start of the packet

Adding an example is ok, but std::end() analogy is not correct. Where std::end points is not actually defined, even for std::vector. And for std::map there is no past-the-end of the last element.
Comment 5 Tom Henderson 2018-07-26 13:11:56 EDT
Created attachment 3152 [details]
revised test case using PrintByteTags ()
Comment 6 Tom Henderson 2018-07-26 13:16:46 EDT
Here is output of the revised test program (revised to use existing PrintByteTags() method):

First packet of 500 bytes tagged:
ns3::MyTag [0-500] v=1

Second packet of 500 bytes tagged:
ns3::MyTag [0-500] v=2

First packet added to end of second packet, new packet of 1000 bytes:
ns3::MyTag [0-500] v=2 ns3::MyTag [500-1000] v=1

Fragment packet back into two 500 byte packets
Fragment 1:
ns3::MyTag [0-500] v=2

Fragment 2:
ns3::MyTag [0-500] v=1


The semantics of GetStart and GetEnd are different which I found to be surprising upon use (but the Doxygen says these are both 'index').  I might more naturally expect that the 1000 byte packet would display the actual span of bytes tagged, such as:

ns3::MyTag [0-499] v=2 ns3::MyTag [500-999] v=1
Comment 7 Alexander Krotov 2018-07-26 13:38:52 EDT
To make it non-ambiguous, just modify the output code to display:

ns3::MyTag [0-500) v=2 ns3::MyTag [500-1000) v=1
Comment 8 Tom Henderson 2018-07-26 13:54:04 EDT
(In reply to Alexander Krotov from comment #7)
> To make it non-ambiguous, just modify the output code to display:
> 
> ns3::MyTag [0-500) v=2 ns3::MyTag [500-1000) v=1

OK, I accept that, but IMO we then need a better description of the return value of GetEnd ().  "Offset from start of packet of the first byte beyond the tagged region"?