Hi Thomas, Thanks for your review and please check inline below for responses. We will incorporate the changes discussed below in the next update.
On Sat, Apr 16, 2022 at 9:31 PM Thomas Fossati via Datatracker < nore...@ietf.org> wrote: > Reviewer: Thomas Fossati > Review result: Ready > > 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 comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-idr-bgp-ls-sbfd-extensions-?? > Reviewer: Thomas Fossati > Review Date: 2022-04-16 > IETF LC End Date: 2022-04-27 > IESG Telechat date: Not scheduled for a telechat > > This document is compact and well written. I think it is ready for > publication. Thank you editors, and all the people involved in its > drafting. I have left a few small editorial suggestions (caveat: I am > completely unfamiliar with the subject matter) in case you may find them > useful. > > ## Section 3, first para: > > OLD > The BGP-LS [RFC7752] specifies the Node NLRI for the advertisement of > nodes and their attributes using the BGP-LS Attribute. > > NEW > BGP-LS [RFC7752] specifies the Node NLRI for the advertisement of > nodes and their attributes using the BGP-LS Attribute. KT> Agree. Will fix. > > > ## Section 3, second para: > > I had to jump a couple of times between the textual definition and the > TLV layout to understand how the two actually match. Specifically, I > think it's the "Length: variable." bit that triggered my confusion. > > So, I have an editorial suggestion that would have saved me from a > (very) mild cognitive dissonance: > > OLD > 0 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Type | Length | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Discriminator 1 | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Discriminator 2 (Optional) | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | ... | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Discriminator n (Optional) | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Figure 1: S-BFD Discriminators TLV > > where: > > o Type: 1032 (early allocation by IANA) > > o Length: variable. It MUST be a minimum of 4 octets and increments > of 4 octets for each additional discriminator. > > o Discriminator n: 4 octets each, carrying an S-BFD local > discriminator value of the node. At least one discriminator MUST > be included in the TLV. > > NEW > 0 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Type | Length=4*n | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Discriminator 1 | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Discriminator 2 (Optional) | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | ... | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > | Discriminator n (Optional) | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > where Type is 1032, Length is 4*n, and n is the number of S-BFD local > discriminator values of the node carried in the TLV. n MUST be at least > 1. > KT> I believe the existing text is quite simple and aligned to existing conventions for BGP-LS TLV description. > > ## Section 3, ignorant hat on: > > I am not sure what is the typical number of discriminators expected in a > TLV? It looks to me that ~65K/4 is a decent magnitude. What is the > expectation in case the number of discriminators is higher than the > maximum allowed by the TLV encoding? Is this a valid scenario? If so, > how is overflow handled? > KT> The overflow is not really valid. The info is sourced from IGPs and there the size for ISIS is much lower while for OSPF is the same as BGP-LS. Therefore, I don't see the the possibility of overflow. > > ## Section 4, Table 1: > > The structure doesn't seem to match the layout of the "BGP-LS Node > Descriptor, etc." IANA registry. In particular: > * The length field seems spurious? > * The IS-IS TLV/Sub-TLV is missing — but I guess this can be skipped, so > not a problem; > * The first column heading should be "TLV Code Point"; > * The Reference column is missing. > > OLD > +---------------+--------------------------+----------+ > | Code Point | Description | Length | > +---------------+--------------------------+----------+ > | 1032 | S-BFD Discriminators TLV | variable | > +---------------+--------------------------+----------+ > > NEW > +----------------+--------------------------+-----------+ > | TLV Code Point | Description | Reference | > +----------------+--------------------------+-----------+ > | 1032 | S-BFD Discriminators TLV | RFCthis | > +----------------+--------------------------+-----------+ > KT> Agree. Will fix. > > ## Section 5, first sentence: > > OLD > The new protocol extensions introduced in this document augment the > existing IGP topology information that was distributed via [RFC7752]. > > NEW > The new protocol extensions introduced in this document augment the > existing IGP topology information that was distributed via BGP-LS > [RFC7752]. > KT> Agree. > > ## Section 6, first para: > > OLD > The new protocol extensions introduced in this document augment the > existing IGP topology information that can be distributed via > [RFC7752]. > > NEW > The new protocol extensions introduced in this document augment the > existing IGP topology information that can be distributed via BGP-LS > [RFC7752]. > KT> Agree > > # Section 5, third sentence: > > Typo: "now encompasses" should be "now encompass" > KT> Agree Thanks, Ketan
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
