Project

General

Profile

Actions

Bug #4403

closed

LpPacket Sequence number field incorrect encoding

Added by Junxiao Shi over 6 years ago. Updated almost 6 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

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.


Related issues 2 (0 open2 closed)

Related to ndn-cxx - Task #4598: Stop accepting NonNegativeInteger as NDNLP sequence numberClosedJunxiao Shi

Actions
Blocks NFD - Task #4564: Release 0.6.2ClosedAlex Afanasyev05/04/2018

Actions
Actions #1

Updated by Junxiao Shi almost 6 years ago

  • Status changed from New to Code review
  • Assignee set to Junxiao Shi
  • Start date deleted (12/19/2017)
  • % Done changed from 0 to 100
Actions #2

Updated by Davide Pesavento almost 6 years ago

What does "fixed-width unsigned integer" mean anyway? Does the spec say how it should be encoded (its endianness for instance)?

Actions #3

Updated by Davide Pesavento almost 6 years ago

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.

Actions #4

Updated by Junxiao Shi almost 6 years ago

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.

Actions #5

Updated by Davide Pesavento almost 6 years ago

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.

Actions #6

Updated by Junxiao Shi almost 6 years ago

Having less branching (one) is always faster than having more branching (four). It will be measurable when executed millions of times.

Actions #7

Updated by Davide Pesavento almost 6 years ago

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.

Actions #8

Updated by Junxiao Shi almost 6 years ago

20180418 NFD call decides that this Change can proceed.

Actions #9

Updated by Junxiao Shi almost 6 years ago

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.

Actions #10

Updated by Junxiao Shi almost 6 years ago

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.

Actions #11

Updated by Junxiao Shi almost 6 years ago

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.

Actions #12

Updated by Davide Pesavento almost 6 years ago

  • 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.

Actions #13

Updated by Junxiao Shi almost 6 years ago

  • Related to Task #4598: Stop accepting NonNegativeInteger as NDNLP sequence number added
Actions #14

Updated by Junxiao Shi almost 6 years ago

will be changed to fixed-width after the upcoming release.

That shall be a separate issue. #4598

Actions #15

Updated by Davide Pesavento almost 6 years ago

Actions #16

Updated by Davide Pesavento almost 6 years ago

  • Status changed from In Progress to Closed
  • % Done changed from 90 to 100

Closing then.

Actions

Also available in: Atom PDF