NFD » ndn-cxx

I'm a bit confused about some of the field declarations relating to Boost Concepts(?). Particularly, I'm confused about the field declarations, like the following (line 98):

typedef /* ValueType=Sequence */ SequenceField;

In particular, what does ValueType mean? I'm guessing it has something to do with Concepts. I've never worked with Boost Concepts before, so foregive me if my question is unclear.

Also, I noticed that the example() function directly creates NdnlpPackets. Splitting Data and creating NdnlpPackets is directly handled by the forwarding daemon and not this API?

Answer to note-6:

ValueType=Sequence is unrelated to Boost.
It refers to an implementation-defined type such that std::is_same<SequenceField::ValueType, Sequence>.

Answer to note-7:

Fragmenting a network layer packet into NdnlpPackets is the responsibility of NDNLPv2 fragmentation feature implementation.
It's not shown at packet format API level.

• Why is this necessary? choose one nesting or the other, having both will only cause confusion IMO.

namespace tlv = ::ndn::tlv::ndnlp2;

• I prefer the name NegativeAck but no big deal.

class Nack; // declared in network layer API

• What is the NackReasonConcrete concept supposed to check?

Answer to note-9:

::ndn::tlv::ndnlp2 namespace is the official spelling.

::ndn::ndnlp2::tlv is an alias to allow NDNLPv2 API and implementation to write just tlv::Element.

NACK is a concept described in several papers.
It's cumbersome to spell out the full title.

NackReasonConcrete is a subclass of NackReason, and also has the TlvType typedef.

NackReason base class is necessary so that when code wants to store the reason without inspecting it, it can be stored as the base class.

Junxiao Shi wrote:

::ndn::tlv::ndnlp2 namespace is the official spelling.

What does official mean? Official regarding to what? Why can't the other one be the official naming since it's more convenient to use?

Answer to note-11:

::ndn::tlv::ndnlp2 is official spelling because all other TLV-TYPE codes (eg. codes used by NFD Management protocol in ndn-cxx) are under ::ndn::tlv namespace, not ::ndn::X::tlv.

Several mixed comments about API and spec

• Why not to call namespace ndn::lp2. ndn::ndnlp2 is a little repetitive.
  Also. Do we really need to emphasize "version 2"? I would just call ndn::lp or ndn::ndnlp

• Agree with Davide about tlv namespace. Is there a single use of ndn::tlv::ndnlp2? For other cases (really just ndn::tlv::nfd and ndn::tlv::security) we may have made mistake. My original intention was to use namespace to disambiguate uses. Either way (tlv::nfd and nfd::tlv) provide disambiguation. If the latter easier to use (and we probably using it), then there is no reason to repeat the error.

• API includes too many repetitions. "Ndnlp" prefix should be omitted in many cases, given it is already in the namespace.

• (for spec only) Can we call "NdnlpHeaderExtension" a "Feature"?

• (for spec. Network NACK) Why don't we call "Nack" as "NackReason" or just "Reason". The description already calls it this way. For "DuplicataNack" and "GiveUpNack" I would lose "Nack" suffix. Why NACK reason should have "Nack" in it's name?

• Why NdnlpPacket is assigned code 100 (0x64)?

• What is field_location_tags::Base ?

• Don't understand why we have such a distinction between NdnlpSequence and all other features. The behavior is exactly the same.

• Should "Field" concept be renamed to "Feature" (and relevant typedefs from xxxField to xxxFeature)? Or, this would be preferred for me, completely remove the suffix.

• We probably should have conditional implementation for (if possible), given it make sense only for features that can be repeated.

  template<typename FIELD>
  std::vector<FIELD::ValueType>
  list();
  
  template<typename FIELD>
  NdnlpPacket&
  add(const FIELD::ValueType& value);
  

• How are you planning to use FieldSet?

• Why tlv::Nack is derived from TagHost? How to use Nack?

Answers to note-13:

See #2763 note-10 for answers to spec questions.

Why not to call namespace ndn::lp2. ndn::ndnlp2 is a little repetitive.
Also. Do we really need to emphasize "version 2"? I would just call ndn::lp or ndn::ndnlp

lp aka "link protocol" is a generic concept, which includes NDNLP and other link protocols.
NDNLP specifically refers to the protocol we are implementing.

2 is necessary because nfd::ndnlp contains the implementation for NDNLP-TLV, and nfd::ndnlp2 namespace will be needed for certain feature implementations.

Agree with Davide about tlv namespace. Is there a single use of ndn::tlv::ndnlp2? For other cases (really just ndn::tlv::nfd and ndn::tlv::security) we may have made mistake. My original intention was to use namespace to disambiguate uses. Either way (tlv::nfd and nfd::tlv) provide disambiguation. If the latter easier to use (and we probably using it), then there is no reason to repeat the error.

Agreed. I didn't know ndn::tlv::nfd was a mistake.

API includes too many repetitions. "Ndnlp" prefix should be omitted in many cases, given it is already in the namespace.

"Ndnlp" prefix appears at:

• NdnlpPacket
• tlv::Ndnlp*

It's unwise to rename NdnlpPacket to Packet, because Packet is too generic.

Constants under tlv namespace should precisely follow the NDNLPv2 spec.

What is field_location_tags::Base ?

This is the base class of the real tags.

It's used to implement the concept check for Field where Field::FieldLocation is asserted to be a subclass of field_location_tags::Base.

Don't understand why we have such a distinction between NdnlpSequence and all other features. The behavior is exactly the same.

NdnlpSequence field is not a header extension.

It must appear first under NdnlpHeader element, before any header extension field.

Should "Field" concept be renamed to "Feature" (and relevant typedefs from xxxField to xxxFeature)? Or, this would be preferred for me, completely remove the suffix.

No. See #2763 note-10: feature and field are distinct concepts.

Although dropping the "Field" suffix makes code look nicer (eg. pkt->get<Sequence>()), there would be implementation difficulties due to name clashing.
For example:

typedef uint64_t Sequence; // value type for NdnlpSequence field
typedef /* implementation-defined */ SequenceField; // field declaration for NdnlpSequence field

Renaming SequenceField to Sequence causes name clashing.

We probably should have conditional implementation for (if possible), given it make sense only for features that can be repeated.

    template<typename FIELD>
    std::vector<FIELD::ValueType>
    list();

    template<typename FIELD>
    NdnlpPacket&
    add(const FIELD::ValueType& value);

This is feasible but unnecessary.

Also note that set is defined to depend on add.

How are you planning to use FieldSet?

NdnlpPacket::wireEncode and NdnlpPacket::wireDecode can iterate over FieldSet, once per distinct FieldLocation, to perform encoding and decoding.

Why tlv::Nack is derived from TagHost? How to use Nack?

tlv::Nack is NOT derived from TagHost.

Nack is derived from TagHost. It's a first-class packet just like Interest and Data.

20150608 conference call reviewed revision 59f6bcc13914b77f45fc27024d5330fea4428f64.

Some comments are:

• The namespace could be just ndn::lp. If someone creates another link protocol, that protocol should find a namespace that doesn't conflict with this.
• Ndnlp prefix of most types should be dropped.
• As part of spec change, Sequence becomes a header field and does not need a separate location tag.

Design is updated according to note-15.

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/a1f731a8ca98b69520e3d3e5f9fbfe019a65f74a

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/fb91230d2168f0b58dd8570a36c4d256432d882d

Clarification added for NackField.

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/92c1ad907c314727ad2af31ba4b7aa978d077f30

FaceId type is eliminated in favor of using uint64_t directly.

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/437a1dcac0ec708bd5e52610ffcffe753e572be8

• major change around NackReason to resolve #2877 note-11 note-13 issues
• divide Nack into NackHeader and Nack classes: NackHeader only contains the header field, Nack binds a header field and an Interest for use in network layer

While I agree in general to dropping the Ndnlp and Nack prefix/suffix from most names, now I fear that the classes Duplicate and GiveUp became too general... maybe add a Reason suffix or something like that?

Do we really need all this std::integral_constant<uint64_t, ...> stuff? why isn't a static constexpr uint64_t ... enough?

Answer to note-28:

Having a static constant field requires the field to be declared in a .cpp, but it's sometimes desirable to avoid the .cpp.

Using the std::integral_constant template looks better than declaring a static method.

https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/e120cac693cb2e21756a1f2d059e1d5e325d283a

This revision is influenced by http://redmine.named-data.net/issues/2877#note-24, with the following differences:

• A reason code is represented by an enum class rather than an integer. It's better to use a strong type.
• nackInfo API is not defined because it's not used by Duplicate and GiveUp.

I don't really like that you completely removed NackReason-specific elements in the spec. Why do you want to modify core parts of the specification (i.e., suddenly adding rules that Nack can actually contain multiple elements and define rules how to process those elements) instead of defining it just once upfront?

When we have the definition in the spec, when we have a new reason with additional elements, we would only need to do a minor modification of the specification and (if we implement in generalized way with NackInfo API as I suggested) we wouldn't even need to modify anything in the library.

Why does NackHeader::setReason() return a Nack? This doesn't make sense to me, as the Nack object also contains an Interest. Without specifying an Interest to this function, the Nack would have an empty Interest.

Why does NackHeader::setReason() return a Nack?

It's a typo due to earlier renaming from Nack to NackHeader.

This is fixed in revision 0abc0f7da30c54014934c54843f32e5c2612aef7.

When reviewing ndn-cxx:commit:29c6b361eb981c76ef3086d2fd8520bb952bc517, I noticed an unintentional consequence of declaring TLV-TYPE codes in ndn::lp::tlv namespace: NDNLP implementation code cannot reference ndn::tlv members with tlv::.

The following snippet does not compile:

namespace ndn {
namespace lp {
namespace tlv {

enum {
  X = 3
};

} // namespace tlv

void
demo()
{
  int a = tlv::Interest; // needs to be ndn::tlv::Interest
  a += tlv::X;
  throw tlv::Error(); // needs to be ndn::tlv::Error
}

} // namespace lp
} // namespace ndn

Shall I change the namespace back to ndn::tlv::lp?

ndn::lp::tlv alias is also disallowed, otherwise the same problem would occur.

RE note-36:

I would favor going back to ndn::tlv::lp. It fits with the style of the other ndn-cxx TLV namespaces, such as ndn::tlv::nfd and ndn::tlv:security.

I prefer the way it is done currently. Explicitly saying ndn::tlv::Error is an indication that it is related to general tlv. If you don't like typing, we can import some of the common elements to ndn::lp::tlv (e.g., Error), but I don't think it is necessary.

I could be ok moving everything to ndn::tlv namespace, but it has its own problems and consequences, which I don't really like.

I believe I found an error at line 414:

template<encoding::Tag TAG>
size_t
CachePolicy(EncodingImpl<TAG>& encoder) const;

Shouldn't this be:

template<encoding::Tag TAG>
size_t
wireEncode(EncodingImpl<TAG>& encoder) const;

Answer to note-43: you caught another typo.

It's fixed in https://gist.github.com/yoursunny/42f44cc308ca0ce9b3ff/05ce27bd979ad26014aa840a973a599d59c05abc, which also adds an example.

Revsion 8bf17c1aa7436a2a1103bb452f52a8617bae0169 makes a small change to Field::encode API to have consistent parameter type with wireEncode.