Bugzilla – Bug 2487
ByteTag scope off-by-one error
Last modified: 2018-07-26 13:54:04 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.
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.
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?
(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; /**
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.
Created attachment 3152 [details] revised test case using PrintByteTags ()
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
To make it non-ambiguous, just modify the output code to display: ns3::MyTag [0-500) v=2 ns3::MyTag [500-1000) v=1
(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"?