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@xxxxxxxxxxxxxxxxxxxx> 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
