Hi Jon, You are welcome. All open issues are about to get closed. Please see [JM] below.
Thanks, Julien Nov. 21, 2017 - jonathan.hardw...@metaswitch.com: > Hi Julien > > Many thanks for the speedy review! Please see a few answers below, marked > with [Jon]. (All other comments are accepted.) > I will hold the document mark-ups until WGLC ends. > > Cheers > Jon > > > -----Original Message----- > From: Julien Meuric [mailto:julien.meu...@orange.com] > Sent: 21 November 2017 17:08 > > Hi, > > Thank you for this new version of the I-D, it has greatly improved and > clarifies former loose zones. Please find my review below. > > ------ > Abstract > --- > - s/traffic engineering paths (TE paths)/Traffic Engineering paths (TE paths)/ > - I wonder about the expansion of "TE path" above: why not "Engineered" > instead of "Engineering"? (This is global to the I-D, and beyond... RFC > Editor's list includes both.) > - s/label switched paths (LSPs)/Label Switched Paths (LSPs)/ > ------ > Status > --- > - "https://"; was introduced in -05, but has now disappeared. > ------ > 1. Introduction > --- > - s/Path Computation Element Protocol/Path Computation Element communication > Protocol/ > > - OLD > path setup type needs to be either explicitly indicated or implied in the > appropriate PCEP messages (when necessary)... > NEW > path setup type needs to be explicitly indicated in the appropriate PCEP > messages, unless RSVP-TE type is meant (omission implying this type)... > ------ > 3. Path Setup Type Capability TLV > --- > - s/Initialization phase/initialization phase/ > - I though the discussion on the list was about using a bitmap to identify > supported PSTs: any reason why it is now a list of raw octets? > > [Jon] We did discuss a bit field on the list. The PST field is an 8-bit > value, so a naive implementation of a bit field would use 32 bytes. This > seems wasteful given we only have two values defined so far (possibly 3 with > the up-coming IPv6-SR draft). I then looked at some schemes to shorten the > bit field, e.g. by truncating it, but the result seemed more complicated to > encode than just listing the path setup types. (I also didn't much like > having a PST known both by a value and by a bit field position.) So I opted > to list the PSTs. IMO it's not a problem given the low number of PSTs we > expect (could be a case of famous last words!) What do you think of this > tradeoff? [JM] I do not have any strong opinion on this. If noone objects, it is reasonable to go that way from where we stand. Have you considered a more explicit figure, e.g. the following? +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | PST#1 | PST#2 | Padding | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ // (variable size) // | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > > - The definition of length and padding mixes the words "octet" and "bytes", > depending on the field (probably to due to text coming from RFC 5440). > Consistency would be welcome (the comment appears to be applicable beyond > this section). > > [Jon] Let's standardize on bytes, per RFC 5440. [JM] OK > > ------ > 4. Path Setup Type TLV > --- > - Figure 2 explicitly includes the codepoint for the "Type" (28), the > "Length" field should be treated similarly (4). > - The last sentence of section 4 puzzles me. If the PATH-SETUP-TYPE TLV is > not supported, the PCEP peer cares little if it was "recognized" or not. If > both sub-cases are commonly handled by ignoring, an implementation always > including the RSVP-TE PST will be able to interwork with an implementation > knowing about the TLV without actually supporting it; the current text turns > this situation into an error. > (Note also that RFC 5440 does not distinguish unrecognized and unsupported in > TLV processing rules.) > > [Jon] I think you are right. The same also applies to the final sentence of > section 3, I believe. [JM] I agree. Good catch! > > > - In case my previous comment does not fly, 3 more nits: > * s/recognizes the TLV but does not support the TLV/recognizes the TLV but > does not support its use/ > * s/send PCErr/send a PCErr message/ > * Error-Type is 2, would not 4 fit better? > ------ > 5. Operation > --- > - s/Initialization phase/initialization phase/ > - s/MUST infer/MUST consider/ [explicit => nothing to infer] > - The text says multiple times "unless the intended PST is RSVP-TE, in which > case it MAY omit the PATH-SETUP-TYPE TLV". This is inconsistent with section > 4: "It is RECOMMENDED that a PCEP speaker omits the TLV if the PST is > RSVP-TE." Please choose between MAY and SHOULD, and align. > > [Jon] I think our intent is MAY, so we can reword the text in section 4 to "A > PCEP speaker MAY omit the TLV if the PST is RSVP-TE." [JM] OK > > ------ > > Cheers, > > Julien > _______________________________________________ Pce mailing list Pce@ietf.org https://www.ietf.org/mailman/listinfo/pce
