Hi Danny, Thanks for your comments. See below.
On Wed, May 4, 2016 at 10:27 AM, McPherson, Danny <[email protected]> wrote: > > Hello, > > 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-trill-ia-appsubtlv-07.txt > Reviewer: Danny McPherson > Review Date: May 4, 2016 > Intended Status: Proposed Standard > > > Summary: > > I have some minor concerns about this document that I think should > be resolved before publication. > > > Comments: > > I believe the draft is technically sound, however, the quality and > readability needs a bit more work, particularly as it relates to > introduction of new terms, and consistent application and use of all > terms. There are also some general error handling and encoding > issues that need to be given consideration. > > > Major Issues: > > I have no “Major” issues with this I-D. Thanks. > Minor Issues: > > 1. ERROR HANDLING: There are a number of places in the document > where it discusses the receipt of malformed, badly encoded, > non-matching, or corrupt messages, and the advice is to either > [silently] discard or ignore the messages. Some general guidance > should be given here to enable operational diagnosis of any issues > that may result in temporal or persistent problems, where logging > and other actions should occur. Some aspects of this might leverage > the OAM Framework efforts, although it appears much of the TRILL > work leaves this to the implementer. In the IETF context "silently discard" means that there is no on-the-wire message sent. It says nothing about whether or not counters are kept of such condition or errors are logged. A suggestion to log such events and/or keep such counters can be added. > 2. When using “Nickname” it would be useful to define the encoding > as an unsigned 16-bit integer, or just reference "as specified in S > 3.7 of RFC6325”. OK. Will add the reference. > 3. The inclusion of the “TLV” acronym in the "APPsub-TLV” TLV name > seems loose and redundant to me, as opposed to “APPsub TLV” or > similar. This comes from RFC 6823, Section 3.2, which says that sub-TLVs that go inside the GENAPP TLV "are refrred to as APPsub-TLVs". > 4. Inconsistent use of “Interface Address APPsub-TLV”, “IA > APPSub-TLV”, “Interface Address APP-subTLV”, and “AppsubTLV” makes > it seem like you’re talking about different things. OK - that should be made more consistent, probably standardizing on "IA APPsub-TLV". > 5. The use of “sub-sub-TLV” seems a bit loose and sloppy to me as > well, and should be cleaned up. E.g., S 5.2 “IA Appsub-TLV > Sub-Sub-TLVs SubRegistry" You don't like "sub-sub-TLV"? Seems like, strictly speaking, you have IS-IS PDUs which contain TLVs. Then some TLVs can contain sub-TLVs. (The GENAPP TLV is the only one that occurs to me with a special name for its sub-TLVs, namely APPsub-TLVs.) and some sub-TLVs can contain a further nested level, which it seems to me to be precise and logical to call sub-sub-TLVs. (I am not aware of any requirement for any more deeper nesting in a use of IS-IS.) So, would you prefer that what are called sub-sub-TLVs in this document just be called "sub-TLVs" (which I agree they are) resulting in two different levels with the same name? While there might be some errors in their use in this draft, the mere use of APPsub-TLV and sub-sub-TLV for the two levels does not seem "loose and sloppy" to me... > 6. Only one of the “Figures” is labeled / captioned OK. All the principal figures should be labeled. (I don't think cases where there is a small, indented figure that just expands part of a principal figure and appears shortly after the principal figure need to be captioned.) So, the initial figures in Sections 3.1, 3.2, 3.3, and 3.4 would have Figures numbers and captions added. > 7. The use of “Address Sets” and “Address Sets Ends” makes it a bit > hard to read when used in sentences. Perhaps an acronym for each, > or hyphenating/underscoring them would make it more readable. OK - I'll see what I can do. > 8. S 3.4 the 2-byte “Type” value in the diagram should be > “TOPOLOGY”, not “DATALEN”. Thanks for noticing this error. > 9. I noticed that Radia was a co-author until the last revision, and > now she doesn’t even exist in the Acknowledgements section. While > no explanation is required here, I did find this a bit odd. I think her listing as an author was in error. > 10. IANA Considerations: Some guidance from the IANA folks on the > formatting of this section might be in order. It’s not as clear as > it could be about what their instructions are here. There are some improvements that could be made. In inverse order, Section 5.3 looks fine. In Section 5.2, "Available" should be changed to "Unassigned" as that is the preferred IANA term. Section 5.1 is talking about assignments that have already happened and looks OK as far as the table of values goes; however, the material after the first sentence after that table seems inappropriate in an "IANA Considerations" section and should, perhpas, be in a new "Processing Address Sets" section. > 11. S 2: It’s unclear to me if the “Confidence” value of 255 “being > treated as if it was 254” is inline with RFC6325 S 4.8.1 guidance? The idea is that local configuration or learning should be able to override address reachability received through network messages. Thus such information, when manually configured, defaults to have confidence 255. RFC 6325 Section 4.8.1 just says that information learned via ESADI will have a confidence of from 0 to 254 but don't actually say what to do if it is recreived as 255. This is updated by Section 6.2 RFC 7357, 1st paragraph, that makes it clear that a received value of 255 is just treated as if it was 254. Thus it is consistent with these prior RFCs to the IA APPsub-TLV draft to give this rule for handling the value 255 in the Confidence field of IP APPsub-TLVs. > 12. In general, I agree there appear to be no new Security > Considerations here. I do not believe Asymmetry will be an issue > with the forged packet discard issue although some consideration of > this might be in order (or perhaps simply a reference to SAVI or > other work here). I wonder if some consideration should be given to > broader disclosure of reachable layer 2 addresses here, but that > seems a bit reaching as well. > > > Nits: > > 1. Abstract & Introduction: s/by-pass/bypass/ OK. > 2. S.2: s/Data Label is reachable from /Data Label are reachable/ "... inteface ... is reachable ...", so I think "is" is correct but I'll see if I can re-word this sentence. > 3. A reference for the first use of AFN would be useful, perhaps to > the IANA registry. OK. > 4. Expressing TBD code points in [ ] brackets might help with > readability as well OK. > 5. S 3.2 “if the Length is 0 or 1 or less” — not sure the “or less" > is necessary? OK, the "or less" should be removed. Thanks, Donald =============================== Donald E. Eastlake 3rd +1-508-333-2270 (cell) 155 Beaver Street, Milford, MA 01757 USA [email protected] _______________________________________________ trill mailing list [email protected] https://www.ietf.org/mailman/listinfo/trill
