Hi all, I think the updated module is looking pretty good. I just have one small piece of feedback to share.
According to RFC 8362 Section 3.3, the Attached-Routers TLV doesn't support Sub-TLVs. Therefore, I believe it's necessary to remove the "sub-tlvs" list from the "attached-router-tlv" container. In the same container, the "Adjacent-neighbor-router-id" leaf-list is capitalized when it shouldn't. Regards, Renato. Em ter., 27 de jun. de 2023 às 20:40, Mahesh Jethanandani < mjethanand...@gmail.com> escreveu: > Acee/Yingzhen, > > Thanks for addressing my comments. > > On Jun 27, 2023, at 1:23 PM, Yingzhen Qu <yingzhen.i...@gmail.com> wrote: > > Hi Mahesh, > > We just uploaded version -17 and added a configuration example. Please let > us know if you have any other comments. > > Thanks, > Yingzhen > > On Mon, Jun 26, 2023 at 1:44 PM Acee Lindem <acee.i...@gmail.com> wrote: > >> Hi Mahesh, >> >> Thanks for the review - a lot of good comments. See inline and -16 >> version. >> >> > On Jun 15, 2023, at 5:18 PM, Mahesh Jethanandani via Datatracker < >> nore...@ietf.org> wrote: >> > >> > Reviewer: Mahesh Jethanandani >> > Review result: On the Right Track >> > >> > Document reviewed: draft-ietf-lsr-ospfv3-extended-lsa-yang >> > >> > Status: On the right track >> > >> > I have marked it as On the Right Track, because of some of the points >> discussed >> > below. >> > >> > Summary: >> > >> > This document defines a YANG data model augmenting the IETF OSPF YANG >> model to >> > provide support for OSPFv3 Link State Advertisement (LSA) Extensibility >> as >> > defined in RFC 8362. OSPFv3 Extended LSAs provide extensible TLV-based >> LSAs for >> > the base LSA types defined in RFC 5340. >> > >> > Nits >> > >> > Please add a section on Instructions to RFC editors stating what they >> should do >> > with references such as RFC XXXX. >> > >> > It would be nice to have some consistency between having description and >> > reference statements start on a new line or on the same line as the >> statement. >> > Right now, they are all over the place. >> > >> > Some of the descriptions are very cryptic. E.g. >> > >> > leaf forwarding-address { >> > type inet:ipv4-address; >> > description >> > "Forwarding address"; >> >> I updated the ones that were brief and cryptic. Note that you almost have >> to have knowledge of RFC 5340 and RFC 8362 to understand the encodings. >> >> >> > >> > s/Description/description in the YANG model. Actually, I was surprised >> that >> > pyang did not complain, but yanglint did. >> > >> > libyang err : Invalid character sequence "Description", expected a >> keyword. >> > (Line number 318.) libyang err : Parsing module >> "ietf-ospfv3-extended-lsa" >> > failed. YANGLINT[E]: Parsing schema module >> > "ietf-ospfv3-extended-...@2023-06-08.yang" failed. >> >> >> Fixed - I’m surprised pyang didn’t complain as well. >> >> >> > >> > s/Addrss/Address/ >> >> Fixed. >> >> >> > >> > s/E-/Extended / in all descriptions. >> >> When referring to the actual LSAs, it is should be “E-“. For example, >> E-Router-LSA. In other cases, it is spelled out. See RFC 8362. >> >> >> > >> > Comments: >> > >> > The grouping such as ospfv3-e-lsa-as, ospfv3-e-lsa-area, >> ipv6-fwd-addr-sub-tlv >> > etc. are used in one place only. Is there a reason why this has not >> been pulled >> > inline where it is used? Did not check for all groupings, but if there >> is only >> > one use of them, ideally they should be inlined. >> >> I consolidated these for the link, area, and AS scoped LSDBs. I left the >> fowarding-address Sub-TLV in its own grouping consistent with the other >> Sub-TLVs. >> >> >> >> > >> > No need to repeat parent name in the child. Just length will do in the >> > following. See Section 4.3.1 of RFC 8407. E.g. >> > >> > container route-tag-sub-tlv { >> > description >> > "Route Tag Sub-TLV"; >> > leaf route-tag-sub-tlv-length { >> >> Fixed. >> >> >> > >> > Why a double -- in container unknown--tlv {? >> >> Fixed. >> >> > >> > A pyang compilation of the model with —ietf and —lint option was clean. >> > >> > There are no examples of configuration instance data in the draft. It >> would be >> > helpful not only to validate the model, but also help folks who want to >> use the >> > model. >> >> There are only two booleans that are config=true. We can look at this >> though. >> >> > >> > A idnits run of the draft reveals a few issues. Please address them. >> > >> > Checking boilerplate required by RFC 5378 and the IETF Trust (see >> > https://trustee.ietf.org/license-info): >> > --------------------------------------------------------------------- >> > >> > No issues found here. >> > >> > Checking nits according to >> > https://www.ietf.org/id-info/1id-guidelines.txt: >> > --------------------------------------------------------------------- >> > >> > No issues found here. >> > >> > Checking nits according to https://www.ietf.org/id-info/checklist : >> > --------------------------------------------------------------------- >> > >> > No issues found here. >> > >> > Miscellaneous warnings: >> > --------------------------------------------------------------------- >> > >> > == The copyright year in the IETF Trust and authors Copyright Line >> > does not match the current year >> > >> > == Line 1266 has weird spacing: '... allows a rou...' >> > >> > -- The document date (October 17, 2019) is 1337 days in the past. >> > Is this intentional? >> > >> > Checking references for intended status: Proposed Standard >> > --------------------------------------------------------------------- >> > >> > (See RFCs 3967 and 4897 for information about using normative >> > references to lower-maturity documents in RFCs) >> > >> > == Outdated reference: draft-ietf-bfd-yang has been published as RFC >> > 9127 >> > >> > ** Downref: Normative reference to an Experimental RFC: RFC 1765 >> > >> > ** Downref: Normative reference to an Experimental RFC: RFC 4973 >> > >> > ** Downref: Normative reference to an Informational RFC: RFC 5309 >> > >> > ** Downref: Normative reference to an Informational RFC: RFC 5714 >> > >> > ** Downref: Normative reference to an Informational RFC: RFC 6987 >> >> >> These idnits are fixed. >> >> >> Thanks, >> Acee >> >> >> > >> > Summary: 5 errors (**), 0 flaws (~~), 3 warnings (==), 1 comment >> > (--). >> > >> > >> > >> >> > > Mahesh Jethanandani > mjethanand...@gmail.com > > > > > > > _______________________________________________ > Lsr mailing list > Lsr@ietf.org > https://www.ietf.org/mailman/listinfo/lsr > -- Renato Westphal
_______________________________________________ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr