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 <noreply@xxxxxxxx> 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-lsa@xxxxxxxxxxxxxxx" 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
> (--).
>
>
>
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
