NFD » ndn-cxx

With Packet03Transition permitting unrecognized non-critical TLV elements, this problem becomes acuter: what does operator== mean for an object that represents a TLV element?
It could be defined as either:

• Equality of semantics. Under this choice, MetaInfo("1400"_block) equals MetaInfo("1403 180100"_block) because they are semantically equivalent: the former omits ContentType and accepts its default value, while the latter includes ContentType with the default value.
• Equality of TLV encoding, excluding unrecognized elements. Under this choice, MetaInfo("1400"_block) does not equal MetaInfo("1403 180100"_block). MetaInfo("1403 180100"_block) equals MetaInfo("1405 F000 180100"_block) where F000 is ignored.
• Equality of TLV encoding, including unrecognized elements. Under this choice, MetaInfo("1400"_block) and MetaInfo("1403 180100"_block) and MetaInfo("1405 F000 180100"_block) do not equal one another, despite they all have the same semantics.

This would be a design choice of ndn-cxx library and should be applied to all network-layer TLV element classes in the library. It is not a protocol design question.

In my opinion, it should be the third option (I believe it is now). Semantics is kind of irrelevant here, as you cannot interpret the semantics of application-defined elements.

As for the behavior. I think it is fine as is. It's basically a spec problem. The default value should not have had the explicit encoding, as it effectively has 2 different states representing the same thing.

There's also option 4: remove operator== and operator!= for these classes.
I doubt there are many use cases that require them, beyond low-level wire encoding comparison (essentially a memcmp) that can easily use Block's operators.

In https://gerrit.named-data.net/c/ndn-cxx/+/5505 (still WIP) I have:

• Removed operator== and operator!= from those classes where they are ambiguous or not well-defined:
  • Interest: use matchesInterest and matchesData instead
  • MetaInfo: definition is unsound/inconsistent, see issue description and note-1
  • Data: because it includes MetaInfo and suffers from the same problems, also I don't see many valid use cases
  • Link: definition was "inherited" from Data, hence it wasn't taking the delegation list into account and was giving wrong results
• Kept the operators in the following cases, all of which have clear/intuitive semantics and/or have widespread usage in several codebases:
  • Block
  • Name and Component
  • Delegation and DelegationList
  • KeyLocator (this is debatable, and will probably need an update for v0.3 evolvability rules)
  • AdditionalDescription and ValidityPeriod
  • FaceUri
  • All types implementing the NFD management protocol: they have consistent definitions and somewhat clear semantics, also if we're going to rewrite the protocol there's no point in changing them now
• Left Exclude and Selectors alone because they're obsolete.
• I'm not sure what to do with:
  • PrefixAnnouncement
  • SignatureInfo
  • Signature (are we going to deprecate/remove this class?)
  • Certificate: this "inherited" the operators from Data, being a subclass, although I'm not sure if that made sense. In any case, operator== between certificates seems to be used in a few places, including ndnsec list; is there a sensible definition of equality between two certificates?

PrefixAnnouncement can have operator< and std::hash defined on the announced name, ignoring other fields. Its equality operator then derives from operator<. This allows putting PA into a collection.
Same applies to KiteRequest and KiteAck.

SignatureInfo and Signature do not need compare operators.

Every Certificate is usually signed.
Its operator< should compare full name of the Data packet; if full name is unavailable (such as a default-constructed object), use its name.
Its operator== is derived from operator<, and is equivalent to a wire encoding comparison if wire encoding exists.

The above answers are written without consulting existing implementations of compare operators, and thus are uninfluenced by such.

Junxiao Shi wrote:

PrefixAnnouncement can have operator< and std::hash defined on the announced name, ignoring other fields.

operator< and std::hash are out of scope for this issue.

Its equality operator then derives from operator<.

Ok, but note that this is different from the current definition.

SignatureInfo and Signature do not need compare operators.

I tend to agree.

Every Certificate is usually signed.
Its operator== is derived from operator<, and is equivalent to a wire encoding comparison if wire encoding exists.

If it's going to be equivalent to wire encoding equivalence then I'm not going to define an operator==. Users can call .wireEncode() and compare the returned Block, if that's what they want.

5505,3

https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/562316480
There are build failures in NFD, repo-ng, NLSR, ndncert, ndns.

NFD breaks on SigningInfo. Its equality operators should stay. It's not mentioned in note-5.
NLSR, ndncert, ndns break on Certificate. repo-ng breaks on Data. These usages should compare wireEncode or name instead.

Junxiao Shi wrote:

5505,3

https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/562316480
There are build failures in NFD, repo-ng, NLSR, ndncert, ndns.

NFD breaks on SigningInfo. Its equality operators should stay. It's not mentioned in note-5.

SigningInfo's operator== uses SignatureInfo's operator==. The latter is gone so the former had to go as well. One alternative is to keep the operator for SignatureInfo and give it clear semantics, for instance we could compare the SignatureType and KeyLocator and ignore any other TLVs. Another alternative is to compare the .wireEncode() of the SignatureInfo embedded in SigningInfo.

NLSR, ndncert, ndns break on Certificate. repo-ng breaks on Data. These usages should compare wireEncode or name instead.

Yes. The question is whether comparing the name is correct and sufficient or we need to compare the full name (or .wireEncode() which is essentially equivalent).

NFD breaks on SigningInfo. Its equality operators should stay. It's not mentioned in note-5.

SigningInfo's operator== uses SignatureInfo's operator==. The latter is gone so the former had to go as well. One alternative is to keep the operator for SignatureInfo and give it clear semantics, for instance we could compare the SignatureType and KeyLocator and ignore any other TLVs. Another alternative is to compare the .wireEncode() of the SignatureInfo embedded in SigningInfo.

The problem comes from this constructor:

  /**
   * @param signatureInfo A semi-prepared SignatureInfo which contains other information except
   *                      SignatureType and KeyLocator.  If SignatureType and KeyLocator are
   *                      specified, they may be overwritten by KeyChain.
   */
  SigningInfo(SignerType signerType = SIGNER_TYPE_NULL,
              const Name& signerName = getEmptyName(),
              const SignatureInfo& signatureInfo = getEmptySignatureInfo());

This SignatureInfo was added in https://gerrit.named-data.net/c/ndn-cxx/+/2116/8/src/security/signing-info.hpp#111 to "handle more settings". A quick grep reveals that the current usage is limited to certificate issuance.
Ideally, SignatureInfo embedded in SigningInfo should be compared without SignatureType and KeyLocator elements, since these would be overwritten. However, comparing SignatureInfo::wireEncode() would be sufficient for current use case.

I've decided to split this task in (at least) two commits.

https://gerrit.named-data.net/c/ndn-cxx/+/5505 is the first part. It removes the operators for Interest, MetaInfo, and Signature.
Only a few NFD's unit tests are broken by this change, and those are easily fixed by https://gerrit.named-data.net/c/NFD/+/5666

https://gerrit.named-data.net/c/ndn-cxx/+/5665 is the second part and contains the more "complicated" removals. It's still a WIP so don't look at it yet.