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 > <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" 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 > > <https://trustee.ietf.org/license-info>): > > --------------------------------------------------------------------- > > > > No issues found here. > > > > Checking nits according to > > https://www.ietf.org/id-info/1id-guidelines.txt > > <https://www.ietf.org/id-info/1id-guidelines.txt>: > > --------------------------------------------------------------------- > > > > No issues found here. > > > > Checking nits according to https://www.ietf.org/id-info/checklist > > <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
