Hello Les, thanks for your review. Please find detailed replies inline.
Cheers, Steven & Markus > Major Issues: > > > > My biggest concern is that the document - and its companion HNCP - are not > yet mature enough to be doing last call. Please note that DNCP does not depend on nor reference HNCP in any way. It is a separate draft and it can and should be used not only by HNCP. Furthermore while DNCP has been submitted to the IESG, HNCP has not and is still in WGLC. Therefore I'd kindly ask you to keep issues separate from one another. > What is being defined here is a > "state synchronization protocol" which is used within the context of a > "parent protocol" (most interestingly a routing protocol for the homenet > context) and which depends upon another configuration protocol > (presumably HNCP) to fully define the behavior. I don't believe this to be an accurate summary. Again DNCP does not depend on HNCP, rather the reverse is true. DNCP even defines some extensions that are not used or intended to be used in HNCP. Neither of the two protocols depend on any routing protocol for correct operation. DNCP does not even mention routing anywhere in the draft. HNCP in fact enables routing protocols to operate in typical home network scenarios. > Judging from the review comments provided by others (notably Thomas Clausen's > detailed review) and the continued discussion on the mailing list it has not > yet been demonstrated that the specification is clear enough and robust enough > for implementations to meet all the requirements and interoperate. > > This is not to suggest that you are on the wrong track - but given the > dependencies pushing this to last call seems - to put it politely - > very "ambitious". I would prefer to see more implementation experience before > the document moves to a state where it is presumed to be complete. We addressed Thomas' comments to the best of our knowledge. Please let us know, if any of the issues still remain in the current revisions. Also, a second independent and interoperable implementation of DNCP and (parts of) HNCP exists: https://github.com/jech/shncpd Comments of the author regarding clarity of the draft are already integrated in the current revisions, some more are queued for the next. > I still have some trouble calling this a protocol. This is more of a process - > or part of a process - which comprises a routing protocol. Again "comprises a routing protocol" is simply incorrect. > The process defined > here serves to support reliable distribution and synchronization of "state" > in an efficient manner under a limited set of conditions. I don't want to > quibble too much about the term "protocol" - but I would prefer something > like: > > "a generic set of procedures which - when supplemented by a specific profile - > define a means of maintaining state synchronization" It is very hard to find a wording for this that everyone agrees with. For now it was changed to “DNCP is an abstract protocol, that must be combined with a specific profile to make a complete implementable protocol.” based on a suggestion in Ole Troan’s review. > Section 2 Terminology > > > The term "neighbor" is not defined - but used frequently in the document. > The term "peer" is defined as: > "another DNCP node with which a DNCP node communicates using a particular > local and remote endpoint pair." > > What I am used to is that the definition above for "peer" is usually > associated with the term "neighbor", whereas the term "peer" is more generic - > it is associated with a node in the network which performs the same functions > in the protocol - but is not necessarily a neighbor. The terms "peer" and "neighbor" are used relatively interchangeably in this document. The terminology has been adapted to reflect that. > Section 4.5 illustrates why I find this confusing as it says > > "When receiving a Node Endpoint TLV... the remote node MUST be added as a peer > on the endpoint and a Neighbor TLV (Section 7.3.2) MUST be created > for it." > ??? I think this particular case should actually not be confusing, since all terms used here are defined in the Terminology or as TLVs. However, we may have a look at it again. > Section 4.4 - final bullet on Page 11 > > o Any other TLV: TLVs not recognized by the receiver MUST be > silently ignored. > > Does "ignore mean "discard"? (This is one traditional meaning) > > If so this seems inappropriate as it is part of the database sent by > the node and therefore needs to be retained in order to keep a consistent > database. Perhaps "store but do not process" is a more accurate behavioral > description? TLVs that are part of the database are sent within the Node State TLV (Node Data field). Said Node State TLV is mentioned just before "Any other TLV" in the same list and thus not meant to be part of "other TLV"s. The text now states this explicitly. > Section 6.1 Keep-alives > > Are keep alives sent in multicast-listener mode? >From the text in 6.1.2 and > 6.1.3 it seems "no" - but I am not certain. The are no multicast keep-alives in this case, please see next comment as well. > Section 6.2 Support For Dense Broadcast Links > > If a node is in Multicast-listen+unicast mode does it bear any responsibility > for publishing state data in the event the node with highest node identifier > does not have the latest information? I presume yes - but the text does not > discuss this point. Yes, it is implied by "SHOULD treat the endpoint as a unicast endpoint connected to the node that has the highest node identifier". So effectively, the node with lower identifier (multicast-listen aside) treats it as a unicast connection to the node with highest identifier, including state synchronisation, keep-alives etc. Maybe we can explicitly list some of those consequences, if that makes it more clear. > Also, does multicast-listener mode affect the way neighbors are advertised? > It seems not - so what you are preventing w multicast-listener mode is > redundant state updates - but there is no change to the set of neighbors > advertised (N*(N-1))? The initial paragraph states that that the main reason for this extension is to avoid lots of redundant Neighbor TLVs. A later paragraph also states that usage of this extension results in only a subset of the topology being advertised. So I think this point should be pretty clear already. As for the underlying logic, please see the comment before. > Section 6.3 Node Data Fragmentation The extension and thereby this section has been removed as requested by another reviewer. > Section 7 TLVs > It says "padding bytes with value zero"". > > Is "0' a MUST - or is this the usual "SHOULD be transmitted as zero and > ignored on receipt"? It actually says "Padding bytes with value zero MUST be added" so I guess it should be pretty unambiguous already. Making this a non-MUST would make some things like ordering and hashing of TLV sets a bit awkward. > Section 7.2.1 Node Endpoint TLV > When is this sent and how often? This is described in detail in 4.2, a reference has been added. _______________________________________________ homenet mailing list [email protected] https://www.ietf.org/mailman/listinfo/homenet
