Bug #4403
closed
LpPacket Sequence number field incorrect encoding
Added by Junxiao Shi almost 7 years ago.
Updated over 6 years ago.
Description
NDNLPv2 rev24 defines:
Sequence ::= SEQUENCE-TYPE TLV-LENGTH
fixed-width unsigned integer
The spec also recommends:
Bit width of the sequence is determined on a per-link basis; 8-octet is recommended for today's links.
lp::Packet
incorrectly encodes this field as nonNegativeInteger, as evidenced by EncodeFragment
test case.
- Status changed from New to Code review
- Assignee set to Junxiao Shi
- Start date deleted (
12/19/2017)
- % Done changed from 0 to 100
What does "fixed-width unsigned integer" mean anyway? Does the spec say how it should be encoded (its endianness for instance)?
And why don't we change the protocol spec instead of the code? It doesn't seem that Sequence has uses where it may need to be changed in place (thus potentially changing the encoding length), so nonNegativeInteger shouldn't be slower in any noticeable way.
nonNegativeInteger shouldn't be slower in any noticeable way.
It is slower because decoder contains branching instructions. The decoder is still readNonNegativeInteger for now, but I plan to change it to read uint64 after a release.
I bet the difference is not even measurable. And even in the fixed-width case, you still need at least one branch to check that the size is what you expect.
Having less branching (one) is always faster than having more branching (four). It will be measurable when executed millions of times.
Only one branch is ever taken. The difference is in the number of comparisons.
Regardless, we're not having this argument. It would apply to many other things that are in the protocol, some much worse than nonNegativeInteger.
20180418 NFD call decides that this Change can proceed.
just to double check, the spec doesn't mandate a specific endianness, that's also a per-link decision I guess
I see no reason to mandate a specific endianness. ndn-cxx uses big endian to be compatible with prior versions, but other links can choose differently.
NFD's LpReliability assumes Ack is NNI. This is updated in https://gerrit.named-data.net/4693.
Test case for LpReliability is somewhat simplified because Ack numbers don't make a difference in piggybacking calculation.
I’m thinking about a better idea on calculating the size of a piggybacked field:
size_t sizeofAck = lp::Packet::estimateSize<lp::AckField>(ack);
This can avoid the errorprone field size calculation in NFD’s LinkService.
However, I’d rather do this under a different issue.
- Status changed from Code review to In Progress
- % Done changed from 100 to 90
Moving back to "in progress" since the decoder is still using NNI format and will be changed to fixed-width after the upcoming release.
- Related to Task #4598: Stop accepting NonNegativeInteger as NDNLP sequence number added
will be changed to fixed-width after the upcoming release.
That shall be a separate issue. #4598
- Status changed from In Progress to Closed
- % Done changed from 90 to 100
Also available in: Atom
PDF