Just on the subject of 'contact', RFC6087bis currently says
The contact statement MUST be present. If the module is contained in
a document intended for Standards Track status, then the working
group web and mailing information MUST be present, and the main
document author or editor contact information SHOULD be present. If
additional authors or editors exist, their contact information MAY be
present.
so yes, current thinking is that the WG chairs should not be present
unless they are editors in which case they should be listed as such; in
this case, they are not.
Tom Petch
----- Original Message -----
From: "Jürgen Schönwälder" <j.schoenwaelder@xxxxxxxxxxxxxxxxxxxx>
To: <yang-doctors@xxxxxxxx>
Cc: <draft-ietf-pim-yang.all@xxxxxxxx>; <ietf@xxxxxxxx>; <pim@xxxxxxxx>
Sent: Wednesday, December 20, 2017 9:22 PM
Subject: Yangdoctors last call review of draft-ietf-pim-yang-12
> 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.
>
>