Hi Peter, Thanks for the detailed review. See below,
On Thu, Jan 7, 2010 at 3:13 PM, McCann Peter-A001034 < pete.mcc...@motorola.com> wrote: > I have been selected as the General Area Review Team (Gen-ART) reviewer > for this draft (for background on Gen-ART, please see > http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html > <http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html> ). > > Please resolve these comments along with any other Last Call comments > you may receive. > > Document: draft-ietf-trill-rbridge-protocol-14 > Reviewer: Pete McCann > Review Date: 2010-01-08 > IETF LC End Date: 2010-01-11 > IESG Telechat date: unknown > > Summary: Basically ready, a few minor issues & nits. > > Major issues: None. > Thanks! > Minor issues: > > Section 4.1.4: > This text seems to imply that the FCS of the inner, native frame is > removed and only a single FCS covering the encapsulated frame is > generated. However, it is not quite clear to me which behavior > is intended. Could you add text to clarify? > You are correct in your interpretation. I suggest adding, after the first sentence of the first paragarph in 4.1.4, the following: "Thus, when a frame is encapsulated, the original FCS is not included but is discarded." > Section 4.5: > The k trees are ordered from 1 to k, with up to k of the s trees > advertised by RB1 given tree numbers 1 through s, respectively, and > any remaining trees given numbers in order of priority. > Is this worded correctly? Should it say: > The k trees are ordered from 1 to k, with up to s of the k trees > advertised by RB1 given tree numbers 1 through s, respectively, and > any remaining trees given numbers in order of priority. > The wording is certainly confusing. There are a total of k trees campus wide. RB1 can advertise a list of trees to calculate which could be a list of less than k, exactly k, or more than k. s is the number of trees to calculate advertised by RB1. The numbering of a tree is significant for some multipathing computations. There is also a priority ordering of all possible trees, but the list of trees to calculate advertised by RB1 overrides that priority ordering. So, if is k less than or equal to s, then the first k trees advertised by RB1 are calculated and numbered 1 through k. If k > s, then all trees advertised by RB1 are calculated and numbered 1 through s. And an additional (s - k) tree are calculated and numbered s+1 ... through k. Which trees are computed is actually explained above the text you mention. The text you mention is just about tree numbering / ordering. I suggest replacing the paragraph just before the 4.5.1 section heading with the following: "The k trees calculated for a campus are ordered and numbered from 1 to k. In addition to advertising the number k, RB1 might explicitly advertise a set of s trees by providing a list of s nicknames that are the root of the trees. - If s == k, then the trees are numbered in the order of RB1 advertises them. - If s == 0, then the trees are numbered in order of decreasing priority. For example, if RB1 advertises only that k=2, then the highest priority tree is number 1 and the 2nd highest priority tree is number 2. - If s < k, then those advertised by RB1 are numbered from 1 in the order advertised. Then the remainder are chosen by priority order from among the remaining possible trees with the numbering continuing. For example, if RB1 advertises k=4, advertises {Tx, Ty} as the nicknames of the root of the trees, and the campus wide priority ordering of trees in decreasing order is Ty > Ta > Tc > Tb > Tx, the numbering will be as follows: Tx is 1 and Ty is 2 since that is the order they are advertised in by RB1. Then Ta is 3 and Tc is 4 because they are the highest priority trees that have not already been numbered." Section 4.5.5: > The bullets in this section say, "forwarded onto > adjacencies in the nickname1 tree" but don't explicitly > say that you shouldn't forward onto the adjacency on which > the packet arrived. > Yes. Since it seems like it would be a bit verbose to add this qualification in the many cases where there are references to adjacencies in this bulleted list, I suggest the following. Replace the ":" at the end of the first sentence in 4.5.5 with a period and add the following sentence after it: "References to adjacencies below do not include the adjacency on which a frame was received." > Section 7.2: > Have you made arrangements with IEEE to get these values? They > normally charge a fee for new Ethertypes. > We are presently working on the applications, but they have not been submitted. Our understanding is that it is the policy of the IEEE RAC (Registration Authority Committee) to, on a case-by-case basis, grant code points for standards use to standards development organizations free of charge. For one type of allocation, a standards use multicast address, they even have a standard form to use for this purpose: http://standards.ieee.org/regauth/registry_stdgroupmac.html > Nits/editorial comments: > On the items below, unless I comment otherwise, I agree. > Section 3.5: > > To ensure backward compatible safe operation, when Op-Length is non- > zero indicating that options are present, the top two bits of the > first octet of the options area are specified as follows: > > +------+------+----+----+----+----+----+----+ > | CHbH | CItE | Reserved | > +------+------+----+----+----+----+----+----+ > > Figure 3.2: Options Area Initial Flags Octet > > The diagram shows a 6-bit Reserved field, but the text says "the top > two bits ... are specified". The text doesn't mention the Reserved > field after this. Should you specify that Reserved is set to zero > at Ingress, copied on Transit, and ignored on Egress, like you did > in Section 3.3? And in that case say "the first byte"? > The idea is to push as much as possible off to a different draft and only put in the minimum hooks in the main protocol document. That different draft is now a working group document at http://tools.ietf.org/html/draft-ietf-trill-rbridge-options-00 I suggest that, at the beginning of the third paragraph below the above figure, the first sentence be changed so that instead of starting "Options will be further specified in other documents...", it would start "Options, including the meaning of the bits labeled as Reserved in Figure 3.2, will be further specified in other documents...". > Section 3.7.2: > This simplifies end node learning > Missing a period here? There is also a blank line after this one, > is something else missing? > Yes, there is a missing period. But I've checked and nothing else is missing. > Section 4.1.2: > TRILL data frame with the associated VLAN ID and priority placed in > the Inner.VLAN information. > Should it also copy the C bit? > No, as specified above in the draft, the C bit is always zero in TRILL. > Section 4.2.4.3: > in the VLAN for which it is appointed > Missing period? > Yes. > 30 second > SHOULD BE: > 30 seconds > > Section 4.5: > as describe below > SHOULD BE: > as described below > > Section 4.6.2.4: > know unicast > SHOULD BE: > known unicast > > Section 4.8.1: > to not learn > SHOULD BE: > not to learn > I don't actually see anything wrong with the current wording but I'm willing to change it. > Section 4.9.1: > ports an > enabled. > SHOULD BE: > ports are > enabled. > > Section 4.9.2: > There is a reference to Figure 4.7 but the figure in this section is > labeled 4.5. > Good catch, thanks. > Section 6: > Layer 2 bridging in not inherently secure. > SHOULD BE: > Layer 2 bridging is not inherently secure. > > Section 6.2: > although this processing > SHOULD BE: > and this processing > I think the current wording is better. > Appendix B: > on such as link as it > SHOULD BE: > on such a link as it > Thanks! Donald ============================= Donald E. Eastlake 3rd +1-508-634-2066 (home) 155 Beaver Street Milford, MA 01757 USA d3e...@gmail.com
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
