Hi Ketan, thanks for the quick reply and for taking the time to answer my questions.
And happy to see that (at least some of) my suggestions were useful. Good luck with the rest of the reviews! Cheers, t From: Ketan Talaulikar <ketant.i...@gmail.com> Date: Sunday, 17 April 2022 at 18:15 To: Thomas Fossati <thomas.foss...@arm.com> Cc: gen-art@ietf.org <gen-art@ietf.org>, draft-ietf-idr-bgp-ls-sbfd-extensions....@ietf.org <draft-ietf-idr-bgp-ls-sbfd-extensions....@ietf.org>, i...@ietf.org <i...@ietf.org>, last-c...@ietf.org <last-c...@ietf.org> Subject: Re: Genart last call review of draft-ietf-idr-bgp-ls-sbfd-extensions-07 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<mailto: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 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
