Reviewer: Yingzhen Qu Review result: Has Issues I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft. Document: draft-ietf-babel-rfc6126bis Reviewer: Yingzhen Qu Review Date: 1 August 2019 Intended Status: Standards Track Summary: This document describes the Babel routing protocol, and obsoletes RFC 6126 and 7557. Note: both RFC 6126 and 7557 are experimental, the current bis version is standards track. I don’t know the history behind, but just want to point it out. Overall comments: The document is mostly well written. I’m not a native speaker, so I’d leave all language nits to RFC editors. Issues: (the line number is used is from idnits) There are security concerns raised by Secdir review: https://datatracker.ietf.org/doc/review-ietf-babel-rfc6126bis-10-secdir-lc-kaufman-2019-06-28/ The description about packet trailer is in RFC 7557 section 2.5, but not included in the bis version. Considering this document will obsolete RFC 7557, I suppose it should be self-complete, so readers don’t need to go back to RFC 7557 for more information. There are multiple places in the document about TLV/Sub-TLV being “self-terminating”, but I didn’t find what it means. Maybe I’m missing something here? Section 1.1 “unmanaged and wireless environment”, what does “unmanaged” mean here? Section 3.2.3 The interface Hello seqno is changed to outing multicast hello seqno, however there is no description about unicast hello at all. While I was reading it, I kept thinking what if it’s unicast hello? Is there a unicast hello timer needed? From later sections, I realized there are unicast hellos. So I’d suggest add some descriptions to avoid the confusion. Section 3.2.4 There is “the expected incoming Unicast Hello sequence number” and “the outgoing Unicast Hello sequence number”, but it was not mentioned in section 3.2.3 (related with previous comment). Section 3.8.1.1 After a node receives a route request, if the given prefix doesn’t exist in its route table, it MUST send a retraction for that prefix. So my question is whether a node is allowed to send multiple explicit requests for a given prefix? If so, what happens if both retractions and updates are received? Section 4.4 1616 4.4. Sub-TLV Format 1618 Every TLV carries an explicit length in its header; however, most 1619 TLVs are self-terminating, in the sense that it is possible to 1620 determine the length of the body without reference to the explicit 1621 Length field. If a TLV has a self-terminating format, then it MAY 1622 allow a sequence of sub-TLVs to follow the body. This section is about Sub-TLV, however the description language in this paragraph is mainly about TLV. Section 4.5 it says that “an implementation may choose to use a dedicated stateless parser to parse the packet trailer”. Will the packet trailer be able to use the state parser if there are state there useful or just for implementation purpose? Although there is no packet trailer defined at this moment. Section 4.5 1674 parsing a TLV MUST update the parser state even if the TLV is 1675 otherwise ignored due to an unknown mandatory sub-TLV. Section 4.6.5 1805 send a new scheduled Hello TLV with the same setting of the 1806 Unicast flag. If this is 0, then this Hello represents an I’d suggest changing the text to “the same setting of flags” instead of “the same setting of the Unicast flag”, considering the flags will be extended later. Nits: 1049 metric) from a neighbour neigh with a link cost value equal to cost, 1050 it checks whether it already has a route table entry indexed by [neighbour neigh] => [neighbour] Thanks, Yingzhen
