Hi Stewart, Thanks for review. We need to publish a new version of this draft as we have signification changes in our GitHub repository. On 5/3/17, 10:54 AM, "Stewart Bryant" <stewart@xxxxxxxxxxxx> wrote: >Review is partially done. Another review request has been registered >for completing it. > >Reviewer: Stewart Bryant >Review result: Has Issues > >Firstly I should say that I am not an expert in YANG, hence my >suggestion that you may wish to assign an additional reviewer. There is no paucity of YANG reviewers for this document. > >However in reviewing this a number of questions and issues arose: > >2. Overview > > This document defines the following data types: > >SB> Accessibility note - the order of types seems random. It might >SB> be more helpful to the reader if they were, in a systematic order >SB> for example alphabetical and/or dependency order. They are grouped by functionality with functionally similar types being grouped together. This is similar in organization to a C header file. > >======= > > router-id > Router Identifiers are commonly used to identify a nodes in >SB> s/a nodes/nodes/ Will fix in next revision (if not already fixed in the git repository). >======= > > route-target-type > This type defines the import and export rules of Route Targets, >as > descibed in Section 4.3.1 of [RFC4364]. An example usage can >be >SB> s/descibed/described/ > found in [I-D.ietf-idr-bgp-model]. Will fix in next revision (if not already fixed in the git repository). > >======== > >SB> I am surprised that IP multicast addresses are here, but IP >addresses are not. >SB> I would have thought that both should be in the same place. IP/IPv6 addresses are already defined in RFC 6991 and are imported by this module. > >======== > >SB> In some protocols we use the NTP and the 1588 timer types, I >assume >SB> they are defined elsewhere. Nobody has suggested these heretofore. If you provide types and where they would be used, we will strongly consider including them. > >======== > > mpls-label > The 20 bits label values in an MPLS label stack entry, >specified > in [RFC3032]. This label value does not include the encodings >of > Traffic Class and TTL (time to live). The label range >specified > by this type covers the general use values and the >special-purpose > label values. An example usage can be found in > [I-D.ietf-mpls-base-yang]. > >SB> I am surprised that you don't start with label and then define the > >SB> other label definitions in terms of existing definitions. >SB> The obvious order being label, sp-label, gp-label, generalized YANG Identities are very extendable but somewhat limited in semantics. For example, a base identity is “a new globally unique, abstract, and untyped identity.” So, the sp-label identities cannot have a base type of label (RFC 7950). I have wondered why the can’t have a type myself. > >======== > >S/ "This identity represents IPv4 address family.";/"This identity >represents the IPv4 address family."; > >======== > > //The rest of the values deinfed in the IANA registry >SB> s/deinfed/defined/ > >SB> However a question arises, the list stops at mt-v6 >SB> Why do you stop at this point in the IANA list? >SB> >https://www.iana.org/assignments/address-family-numbers/address-family-num >bers.xhtml >SB> It cannot be because some of the later ones are less relevant, as >some of the >SB> included ones are rather rare. One the other hand there are some >later ones that >SB> seem modern and useful. >SB> >SB> Also why do you have types in this list that you do not later >define in detail? I have included the rest of the IANA defined AFs in the GitHub version. There will be no other YANG description of these address families. However, I have embellished these a bit to at least have the full expansion of acronyms. These will be moved to a separate iana-routing-types module in the same draft. > >=========== > >SB> You have generalized label in the list top, but not in >SB> the YANG model itself. It is on page 16. Thanks, Acee > >=========== > >