Thanks for the review Renato - I’ve incorporated both your comments into the latest revision.
Thanks, Acee > On Jun 28, 2023, at 09:23, Renato Westphal <renatowestp...@gmail.com> wrote: > > 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 <mailto: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 >>> <mailto: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 >>> <mailto: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 <mailto: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 >>>> > <mailto: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 <mailto:mjethanand...@gmail.com> >> >> >> >> >> >> >> _______________________________________________ >> Lsr mailing list >> Lsr@ietf.org <mailto: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
