Subject: [ANCP] Review of draft-ietf-ancp-protocol-09

From: Tom Taylor <[email-masked]>
Date: Thu, 15 Apr 2010 21:21:11 -0400

This is my promised review of the ANCP base protocol document, through section 
5.4.2. I also have noted numerous places where trivial editorial changes might 
be desirable. I am dealing directly with the authors on those.

Initial point: the addresses of two of the authors, Norbert Voigt and Swami
Subramanian, are out of date. This could cause trouble come AUTH48, since the
RFC Editor has to hear back from all authors.

(1) Section 1, fifth paragraph:

The introduction talks about reusing GSMPv3 messages. However, it does not
mention that ANCP also introduces new messages, and that extensions to the base
document can introduce further new messages. I'd suggest that this information
be added.

(2) Section 4.1:

(a) Remove the bullet mentioning multicast replication.

(b) Editorial: integrate the last paragraph with the last sentence of the first
paragraph (both talking about GSMPv3). Move the paragraph with the bullets to
the end, providing a neater transition to the next section.

(3) Section 4.4.1, first para:

Editorial: the text: "The format of relevant extensions to port management
message is defined in section" is duplicated.

(4) Section 5, Sub-version:

The RFC Editor's Note should require the change to be made both in the 
Sub-version bullet and in the paragraph following Figure 6. Add the words: "both 
here and in the paragraph following Figure 6" to the RFC Editor's Note.

(5) Section 5, Result:

I would suggest the first sentence be changed to:

"The Result field uses a subset of the values defined for GSMPv3 [RFC3292], plus 
the additional value 0x00. The specific usage of these Result values may be 
specified for individual message types. Where unspecified, the general rule is 
that values 0x00 through 0x02 MUST appear only in requests, and values 0x03 and 
0x04 MUST appear only in responses.

"The Result field values used in ANCP are:"

(6) Section 5, Message Type:

Editorial: should be moved up to follow Sub-Version.

(7) Section 5, Code:

In the first paragraph, change "is set to zero" to "MUST be set to zero".

(8) Section 5.1:

I would find it less confusing if the terminology used for the delimiting header 
for TCP differed from the terminology used for the message body. There would be 
less chance of confusing the "Type" in the header with the message type if the 
following changes were made:

(a) Delete "TLV" in "TLV header". Just call it a header instead.

(b) Call the first field in the header an identifier.  I suggest replacing the 
existing text by:

"Identifier:

    "This two-byte field has the fixed value 0x880C (i.e., the
     same value as GSMP's Ethertype). It identifies the content following the
     header as a GSMPv3 message".

The figure has to be changed to match.

(9) Section 5.2:

Beginning of second paragraph: change "The NAS will set ..." to "The NAS MUST 
set ...".  Two lines below that, change "are exchanged" to "MUST be exchanged". 
The following paragraph also needs occasional rephrasing to use RFC 2119 language.

The paragraph after that has a bunch of "should"s. These probably need to be 
upper case, and the circumstances where one not use the specified settings have 
to be described. If there is no justification for deviating from the recommended 
settings, these have to be MUSTs.

Again, the next-to-last paragraph dealing with adjacency timeout has to be 
modified to use RFC 2119 language. The reason for "SHOULD" for state cleanup is 
perhaps a bit more obvious here, so it's less necessary to specify the exceptions.

(10) Section 5.3:

(a) I repeat the point I made the other day: the type codes for the capability 
"TLVs" do not appear in the IANA registry and and in fact conflict with existing 
TLV codes. I therefore suggest that we simply call these "capability fields" 
rather than "capability TLVs".

(b) This section also needs to be rewritten to use RFC 2119 language.

(c) In the paragraph just below the figure, dealing with Tech Type, the last 
sentence should be deleted. This is actually a memo to the authors to make sure 
the IANA registration is set up, rather than a specification for the implementer.

(d) The text dealing with Capability Type 0x03 should be deleted.

(11) Section 5.4.1.1:

I'm not sure the extension block itself has a TLV structure. It might be better 
to call it an extension block and say that it encapsulates one or more TLVs. 
That affects the title of sub-section 5.4.1.1 as well as the content of the 
first paragraph and the label of the figure below it. In defining the extension 
value you could say explicitly that it is composed of one or more TLVs as 
described for individual message types.

(12) Section 5.4.2:

(a) In the second paragraph, as much as possible, refer to section 5 for the 
setting of individual fields. There is no need to repeat instructions for 
Version, Sub-Version (with its RFC Editor's Note, missing here), "I" bit, 
SubMessage value, or Length.

(b) fifth paragraph: will the Tech Type always be 0x05, or could PON require a 
different Tech Type?

(c) paragraph following Figure 13:

Change "is silently ignored" to "MUST be silently ignored".

(d) Immediately following that paragraph, the text should be expanded a little:

"The following TLVs are defined for use in the PORT UP / PORT DOWN Event message:"

(e) The syntax for the ASCII strings would be clearer if you omitted the 
quotation marks around the complete string, put angle brackets around the 
variable parts, and either quoted or left unmarked the literals. (Remember to 
use &lt; in your XML2RFC source instead of <).

Example: <Access-Node-Identifier> atm <slot/port>:<vpi>.<vci>

You could even go all the way and use ABNF, but I expect that's overkill.

(f) At the end of the description of the Access-Aggregation-Circuit-ID-ASCII 
(bullet 4), there is a "TBD" regarding uplink characteristics. Can that be 
replaced by an internal reference or should it simply be deleted? Actually, the 
whole paragraph seems to be out of place.

(g) At the beginning of the description of DSL Line Attributes (bullet 5), it is 
stated that the TLV is mandatory. I assume it is mandatory when Tech Type is 
0x05, but not otherwise.

(h) Perhaps the description of the contents of the DSL Line Attributes TLV could 
be phrased this way: that the DSL Line Attributes TLV encapsulates a number of 
other TLVs. Refer to Figure 13 and accompanying text to describe their format, 
and don't talk about the assigned type code values ("start from 0x81").

(i) Would it worthwhile, the first time the abbreviation "Kb" appears, to add: 
"(1 Kb = 1000 bits)"? And if you actually mean 1024 bits, you really should add 
this explanation.