I am the assigned Gen-ART reviewer for this draft. The General Area Review
Team (Gen-ART) reviews all IETF documents being processed by the IESG for
the IETF Chair. Please treat these comments just like any other last call
comment. For background on Gen-ART, please see the FAQ at
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>
Document: draft-ietf-lisp-lcaf-16
Reviewer: Peter Yee
Review Date: October 9, 2016
IETF LC End Date: October 4, 2016
IESG Telechat date: October 13, 2016
My apologies for the lateness in getting this review out. Nonetheless, I
hope you will find the comments useful.
Summary: This draft is basically ready for publication as an Experimental
specification, but has some issues that should be fixed before publication.
[Ready with issues]
Major issues: None
Minor issues:
There's a general problem with the definitions of the Length field used in
specific LCAF Types. Sometimes there's a "number + n" format used to
specify all of the fields after the fields covered by "number". But the
definitions get this wrong frequently and indicate that the "Length value n"
covers all of the fields after it, even when the figure shows a number plus
n. An example is on page 14, section 4.5, but there are several of these
mistakes in the document. I would prefer not to use a "number + n" in the
figure, but just label Length as Length in the figure. This may reduce the
number of times that the definition gets it wrong. It's hardly critical to
count of the number of bytes that cover some but not all of the following
fields, especially when they are quite evident in the figure itself, which
is 4 bytes wide.
Page 6, Rsvd2 definition: the definition both says "reserved for future use"
and then says some types actually use it. That sounds like present use.
And to generically say that it should be sent as zero and ignored, but then
to give uses (such as Type 2) for it is confusing. I suggest rethinking
the wording here.
Page 6, Length definition: there's mention of a "Reserved" field that's
included in the minimum length of 8 bytes that are not part of the length
value. Since there are actually Rsvd1 and Rsvd2 fields in the generic
version of the LCAF and sometimes even Rsvd3 and Rsvd4 fields when using
specific Types, it might be better to spell out which reserved fields (Rsvd1
and Rsvd2) are meant here rather than giving the field a summary name that
doesn't actually appear in the format. This is also important because any
Rsvd3 and Rsvd4 fields are included in the Length field, so using a generic
"Reserved" description is ambiguous at best.
Page 13, RTR RLOC Address definition, 4th sentence: The ability to determine
the number of RTRs encoded by the value of LCAF length implies a single
value for AFI = x is required. If so, why not only use one AFI=x value
rather than repeating it for each address? And if there can be different
AFI = x value, then the number of RTRs that are encoded cannot be determined
without parsing through each AFI/address pair.
Page 13, RTR RLOC Address definition, 5th sentence: this sentence gives two
means to indicate that there RTR field values. What's the point of having
two different means of doing so? This just seems to introduce complexity
and increase the chance for implementations that may not handle both schemes
correctly. One scheme ought to suffice. I prefer the Length field method
since it requires fewer bytes transmitted, but either works.
Nits:
General:
Change "AFI encoded" to "AFI-encoded".
Change "AFI based" to "AFI-based".
Change "LCAF format" to "LCAF".
Change "use-case" to "use case". Change "use-cases" to "use cases".
Expand acronyms/initialisms on first use unless they are really well known.
For example: PA (Provider-Assigned).
Change "ignore on receipt" to "ignored on receipt".
Specific:
Page 5, RLOC definition, 2nd sentence: change "a" to "an".
Page 5, generic LCAF figure: the figure looks like a fixed 8 byte block,
while the text preceding it indicates that there are a variable number of
fields of variable length. Perhaps adding an open-ended field to the figure
might make it clearer what is meant.
Page 5, Type definition: change the comma to a semicolon.
Page 6, Rsvd2 definition: replace the space between "Type" and "dependent"
with a hyphen.
Page 6, Length definition, 6th sentence: insert "an" before "encoded".
Page 8, Usage, 1st sentence: change the second "a" to "an".
Page 9, AS Number definition: insert "to" before "either".
Page 10, Longitude degrees definition: change "Value" to "Valid".
Page 12, Length value n definition: insert "ETR" before "UDP".
Page 13, RTR RLOC Address definition, 3rd sentence: change "these" to
"this".
Page 14, section 4.5, 1st paragraph, 2nd sentence: change "an" to "a".
Page 14, section 4.5, 2nd paragraph: I can't parse the sentence as written
and I'm not sure what's the correct fix. Perhaps "when each EID is away
from its"? Or "when all EIDs are away from their"?
Page 14, section 4.5, Source MaskLen definition: give the units of measure
here (be it bytes or bits), for clarity, unless mask lengths are well known
to be only of a specific unit.
Page 15, Source/Subnet Address and Group Address definitions: delete an
extra space before "is" in each definition.
Page 16, Strict bit (S) definition, 1st sentence: change "Rencap" to
"Reencap".
Page 18, Key Count definition, 2nd sentence: change "the" to "of".
Page 18, AFI = x definition: insert two spaces before the second sentence.
Page 19, section 4.8, 1st paragraph, 1st sentence: change "needs" to "need".
Page 20, Length definition: here's another one of those length errors
mentioned in the minor issues section above.
Page 20, Source-ML and Dest-ML definitions: provide the unit of measure in
use.
Page 24, section 4.10.3, 1st paragraph and Length value n definition: these
two statements do not agree with each other. One includes the AFI value in
'n', the other does not.
Page 24, Length value n definition: insert "of the" before "AFI=17".
Page 27, section 5.1, Length value n definition: I'm not sure what qualifies
as "8-byte Application Data fields", but there appear to be 12 bytes before
the AFI and that's reflected in the Length field in the figure. So the
local and remote port ranges are the 8-byte Application Data fields? This
comes back to my minor issue with how Length values are described in the
text and in the figures. Please clarify what's meant here.
Page 28, section 5.2, Length value n definition: here's another length
description error, although of a slightly different ilk.
Page 29, Key Field Num definition: change "the the" to "the".
Page 32, section 5.5, 1st paragraph, 1st sentence: set off "for example"
with commas fore and aft.
Page 32, section 5.5, 1st paragraph, 2nd sentence: insert "a" before "key".
Page 34, L definition: change "layer3" to "layer 3".
Page 34, l definition: change "layer2" to "layer 2".