Hi Lars, Thanks for your review!
On Thu, Oct 20, 2022 at 4:48 PM Lars Eggert via Datatracker < [email protected]> wrote: > Lars Eggert has entered the following ballot position for > draft-ietf-pce-lsp-extended-flags-07: Discuss > > When responding, please keep the subject line intact and reply to all > email addresses included in the To and CC lines. (Feel free to cut this > introductory paragraph, however.) > > > Please refer to > https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ > for more information about how to handle DISCUSS and COMMENT positions. > > > The document, along with other ballot positions, can be found here: > https://datatracker.ietf.org/doc/draft-ietf-pce-lsp-extended-flags/ > > > > ---------------------------------------------------------------------- > DISCUSS: > ---------------------------------------------------------------------- > > # GEN AD review of draft-ietf-pce-lsp-extended-flags-07 > > CC @larseggert > > Thanks to Roni Even for the General Area Review Team (Gen-ART) review > (https://mailarchive.ietf.org/arch/msg/gen-art/J2jEQIJoyAsZEbtU5IK14E3yROA > ). > > ## Discuss > > ### 2119 terms > > This document has some ambiguities that would be clarified by using > RFC2119 terms in a few more places: > > #### Section 3.2, paragraph 2 > ``` > - Note that PCEP peers MAY encounter varying lengths of the LSP- > - ^^ -------- > + Note that PCEP peers MUST handle varying lengths of the LSP- > + ^^^ +++++ > ``` > > #### Section 3.2, paragraph 3 > ``` > - than it currently supports or understands, it will simply ignore the > - ^^^^^^^^^^^ > + than it currently supports or understands, it MUST ignore the > + ^^^^ > ``` > > #### Section 3.2, paragraph 4 > ``` > - than the one supported by the implementation, it will consider the > - ^^^^ ^^^^^^ ^ > + than the one supported by the implementation, it MUST treat the > + ^^^^ ^^ ^^ > ``` > > #### Section 5, paragraph 2 > ``` > - not understood by an implementation would be ignored. It is expected > - ^^^^^ > + not understood by an implementation MUST be ignored. It is expected > + ^^^^ > ``` > > Dhruv: I dont see any issue in using normative MUST. Thanks for pointing them out. > > ---------------------------------------------------------------------- > COMMENT: > ---------------------------------------------------------------------- > > ## Comments > > ### Section 3.1, paragraph 7 > ``` > LSP Extended Flags: this contains an array of units of 32-bit flags > numbered from the most significant as bit zero, where each bit > represents one LSP flag (for operation, feature, or state). > Currently no bits are assigned. Unassigned bits MUST be set to zero > on transmission and MUST be ignored on receipt. > ``` > Should this document give some encoding guidance, e.g., "the LSP Extended > Flags > field SHOULD (MUST?) use the minimal amount of space needed to encode the > flag > bits"? Otherwise one would be free to have trailing 32-bit values with all > flags > zero. > > Dhruv: We can add this sentence with a SHOULD. > ### Section 3.2, paragraph 2 > ``` > The LSP Extended Flags field is an array of units of 32 flags, to be > allocated starting from the most significant bit. The bits of the > LSP Extended Flags field will be assigned by future documents. This > document does not define any flags. Unassigned flags MUST be set to > zero on transmission and MUST be ignored on receipt. Implementations > that do not understand any particular flag MUST ignore the flag. > ``` > What "unassigned flags" are will change as assignments are made, so this > text > would require implementations to (closely) track IANA assignments. Did you > maybe > mean "flags that an implementation is not supporting" instead? > Dhruv: I dont think so! This text is already covered as part of section 3.1 when describing the LSP Extended Flags with "Unassigned bits MUST be set to zero on transmission and MUST be ignored on receipt." The text makes sure that the backward compatibility is maintained when we assign new bits. We can remove the text from 3.2 if it seems confusing! > > ### Section 4, paragraph 2 > ``` > Following the model provided in [RFC8786] Section 3.1, we provide the > following advice for new specifications that define additional flags. > Each such specification is expected to describe the interaction > between these new flags and any existing flags. In particular, new > specifications are expected to explain how to handle the cases when > both new and pre-existing flags are set. They are also expected to > discuss any security implications of the additional flags (if any) > and their interactions with existing flags. > ``` > I think you mean "describe the interaction between these new flags and *all > existing assigned* flags". That still leaves the issue of what to do when > two > documents simultaneously define new flags; that would need to be caught > during > standardization. > > Dhruv: Isn't that business as usual :) The flags are to be defined by standards action and we can track parallel work during normal WG process. Thanks! Dhruv (as a Shepherd) > ## Nits > > All comments below are about very minor potential issues that you may > choose to > address in some way - or ignore - as you see fit. Some were flagged by > automated tools (via https://github.com/larseggert/ietf-reviewtool), so > there > will likely be some false positives. There is no need to let me know what > you > did with these suggestions. > > ### Grammar/style > > #### Section 3.1, paragraph 2 > ``` > or operation, feature, or state). Currently no bits are assigned. > Unassigned > ^^^^^^^^^ > ``` > A comma may be missing after the conjunctive/linking adverb "Currently". > > ## Notes > > This review is in the ["IETF Comments" Markdown format][ICMF], You can use > the > [`ietf-comments` tool][ICT] to automatically convert this review into > individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT]. > > [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md > [ICT]: https://github.com/mnot/ietf-comments > [IRT]: https://github.com/larseggert/ietf-reviewtool > > > >
_______________________________________________ Pce mailing list [email protected] https://www.ietf.org/mailman/listinfo/pce
