Hi Mahesh, Thanks for the good observations. We have updated the model according to your suggestions, with an updated revision https://tools.ietf.org/html/draft-ietf-pim-yang-15. Best regards, - Xufeng > -----Original Message----- > From: Mahesh Jethanandani [mailto:mjethanandani@xxxxxxxxx] > Sent: Tuesday, January 2, 2018 4:22 PM > To: Juergen Schoenwaelder <j.schoenwaelder@xxxxxxxxxxxxxxxxxxxx> > Cc: YANG Doctors <yang-doctors@xxxxxxxx>; draft-ietf-pim-yang.all@xxxxxxxx; > ietf@xxxxxxxx; pim@xxxxxxxx > Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-pim-yang- > 12 > > Few other observations. > > Is the YANG model description statement not supposed to carry the IETF > copyright statement? > > There is already a enum definition for BFD state called “state” in the BFD types > file imported by the module. Why is that not being used in this model? > > > On Dec 20, 2017, at 1:22 PM, Jürgen Schönwälder <j.schoenwaelder@jacobs- > university.de> wrote: > > > > Reviewer: Jürgen Schönwälder > > Review result: Almost Ready > > > > * General YANG Review Remarks > > > > This document depends on a number of other I-Ds. Is it safe to > > process this document while the other documents this document depends > > on are not yet through the process? Who is tracking things and making > > sure that any changes in other documents that may impact the PIM > > document are detected and handled appropriately before publication? > > > > It is generally good style to write complete sentences in > > description statements. Some of the description statements are just > > fractions of a sentence. > > > > I think we do not recommend anymore to list WG chairs in the contact > > statement. > > > > It is sometimes not clear why you define groupings that are only > > used once (and sometimes are likely not reusable since they contain > > relative paths in must expressions). > > > > * Introduction > > > > - Is there a reason why you refer to RFC 6020 and to RFC 7950 in > > sections 1 and 1.2? Why do you need the reference to RFC 6020? > > > > - What are 'wider' management interfaces? If you mention NETCONF, > > why not mention RESTCONF? > > > > - s/Protocol-Independent Multicast (PIM) devices > > /devices supporting Protocol-Independent Multicast (PIM)/ > > > > - So which YANG terminology applies, RFC 6020 or RFC 7950? I > > personally think using YANG 1.1 is pretty safe these days. > > > > * Design of Data Model > > > > - I did not really understand Section 2.5. It seems you are > > duplicating objects for different address families but on some > > systems these duplicate objects must have the same value. If so, > > where would the must expression go and how does an implementation > > add such a must expression? How many of such must expressions > > would there have to be? Did you consider having address family > > independent objects and optionally (controlled by a feature) per > > address family objects that overwrite the independent settings? > > > > * Module Structure > > > > - s/is included/is imported/ > > > > - The tree diagrams are rather long. It would likely help readers if > > the diagram would be split into meaningful units and additional > > text added to describe the units. > > > > Are lists of the form > > > > | +--rw ipv4-rp* [ipv4-address] > > | | +--rw ipv4-address inet:ipv4-address > > > > designed for exensibility? Otherwise, this may be collapsed into > > a simple leaf-list. > > > > - Since I am not familiar with details of PIM, I can't comment on > > the question whether the model makes sense or not. > > > > * PIM base Module > > > > - YANG modules compile cleanly according to the tracker page. > > > > - As said above, consider using YANG 1.1. The ietf-routing module > > actually uses YANG 1.1 so you will need a YANG 1.1 compiler > > anyway. > > > > - Consider adding reference statements to the feature definitions > > in case a feature is described in a protocol specification. > > > > - The value of timer-value is not really clear, you could have used > > rt-types:timer-value-seconds16 directly. > > > > - Why is graceful-restart/duration using an ad-hoc type for 16-bit > > seconds and not timer-value? Is it because infinity and not-set > > does not apply? > > > > - Does a bfd/hello-interval of 'infinity' or 'not-set' make sense? > > > > - More explicit description of bfd/hello-holdtime? Is a choice > > really appropriate (hello-holdtime-or-multiplier)? Can I not have > > both holdtime and multiplier? Perhaps I am just not clear what > > holdtime does... > > > > - Does a bfd/jp-interval of 'infinity' or 'not-set' make sense? > > > > - More explicit description of bfd/jp-holdtime? Is a choice really > > appropriate (jp-holdtime-or-multiplier)? Can I not have both > > holdtime and multiplier? Perhaps I am just not clear what holdtime > > does... > > > > - Please provide more meaningful descriptions: > > > > description "Propagation description"; > > description "Override interval"; > > > > - What is the meaning of the interface augmentation 'oper-status' > > relative to 'oper-status' defined by ietf-interfaces? Is this just > > a duplicate with fewer states? Or is this somehow more specific to > > multicast or PIM packets? In the later case, I think this deserves > > to the be explained in the description clause. > > > > - How do the ip4 and ipv6 addresses relate to ip addresses assigned > > to an interface in ietf-ip? > > > > - What is the meaning of hello-expiration 'not-set'? > > > > - What is the meaning of expiration 'not-set'? > > > > - Is it useful to return the uptime in seconds (which is changing on > > every get that is not in the same second) or could it be an option > > to report the time when something transitioned into the up state > > (which is not changing)? Well, it could be that we are simply used > > to uptime like objects. Anyway, the description of up-time should > > make it clear what exactly is defining the state 'up'. If this > > says up for 5 seconds, what exactly transitioned into an 'up' > > state 5 seconds ago? > > > > - Is the any restriction for dr-priority or is it a full 32-bit > > unsigned number space? In some vendor documentation I saw 0..65535 > > with a default of 1. What do RFCs say? > > > > - I am not sure what the precise meaning of the error statistic > > counters are. What turns an received or sent messages into an > > error message? The description of grouping statistics-error does > > not explain this. Also, if I receive a hello and I later decide > > that this hello must have been an error, is this hello counted > > twice? And what about messages that could not be parsed because > > they were malformed, where are those counted? > > > > - Why is 'pim' a presence container? > > > > - Do comments like 'configuration data nodes' make sense if you > > include config false nodes in the same branch of the tree? > > > > * PIM RP Module > > > > - Does the feature bsr-election-state depend on the feature bsr? > > > > - Should there be a default bsr-candidate/priority? > > > > - Do you need the address/hash-mask-length/priority in > > bsr-state-attributes in an NMDA implementation? > > > > - I _assume_ the bsr-next-bootstrap value has to be interpreted > > relative to the time the value was obtained. What about making > > this an absolute timestamp instead? Well, actually I am not sure > > what the value represents - is it the remaining time interval > > until the next bootstrap will be sent? > > > > - I have no clue what to expect here: > > > > leaf group-policy { > > type string; > > description "Group policy."; > > } > > > > What can I expect the string to contain? > > > > - I am again uncertain how exactly to understand the value of > > rp-candidate-next-advertisement, see similar questions above. > > > > - What are the policy values that can go into this: > > > > leaf policy-name { > > type string; > > description > > "Static RP policy."; > > } > > > > Is the string just an arbitrary name or does it mean something? > > > > - How is this supposed to be used? > > > > leaf policy { > > type string; > > description > > "ACL (Access Control List) policy used to filter group > > addresses."; > > } > > > > What is the meaning of <policy>foo</policy>? > > > > * PIM SM Module > > > > - What is the meaning of a policy-name value? > > > > leaf policy-name { > > if-feature spt-switch-policy; > > type string; > > description > > "Switch policy."; > > } > > > > - What is the meaning of a range-policy value? > > > > leaf range-policy { > > type string; > > description > > "Policy used to define SSM address range."; > > } > > > > * PIM DM Module > > > > - I wonder, would you need an identity for dense mode? > > > > * PIM BIDIR Module > > > > - Remove > > > > /* > > * Typedefs > > */ > > > > - What is the meaning of offer-interval or backoff-interval > > 'not-set'? > > > > * Implementation Status > > > > It seems the trac page pointed to was used to collect information > > about what proprietary implementation support, i.e., it does not > > document to what extend the models defined in this document have been > > implemented. There are some notable differences. For example, it > > seems most counters implemented are 32-bit while most counters in the > > YANG models are 64-bit. How chatty is PIM, are 64-bit counters really > > needed and are implementors willing to move to 64-bit counters? > > > > * Security Considerations > > > > I think this needs to include a bit more details. See > > > > https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines > > > > for the latest template that YANG module authors are expected to > > use. > > > > > > _______________________________________________ > > yang-doctors mailing list > > yang-doctors@xxxxxxxx > > https://www.ietf.org/mailman/listinfo/yang-doctors > > Mahesh Jethanandani > mjethanandani@xxxxxxxxx
