Overall:
- no compiler warnings; passes --ietf check as well
Normative Sections:
sec 1:
- YANG version cited should be RFC 7950, not 6020.
YANG 1.1 is used in this document.\
sec 4: Relationship with RFC 7317
- this section should say how overlapping configuration
objects interact. Does setting 1 field (e.g., /system/ntp/enabled)
change the other field at the same time (e.g., /ntp/enabled)
It seems the intent is to ignore and deprecate /system/ntp if
/ntp is implemented. This section should explain.
sec 5:
- this section is supposed to have citations for every imported
YANG module used in the ietf-ntp YANG module
YANG Module Issues:
1 - Indexing used in /ntp/associations list
key "address local-mode isConfigured";
a) will there really be multiple instances per address for
different association-modes? The list description-stmt
should explain.
b) There can be configured values (isConfigured key) and learned
entries for the same address and association-modes? Why isn't
NMDA used instead? (i.e. intended and operational datastores
used instead of the isConfigured list key?)
2 - Purpose of /ntp/access-rules
There is no explanation for what it means to configure an entry
here that points at an ACL entry in /acls/acl; The description
statement needs to specify the details. Doesn't the ACL module
just work? How does the /ntp/access-rules/access-rule list
change behavior?
3 - Reinvent Key Management
It does not seem like a good idea for every protocol to
duplicate key management functionality. The draft should
explain why other YANG modules related to this work are not
relevant here
4 - Reference statements
There are no reference statements used in any objects or typedes
Definitions which are intended to match a definition in NTP
should include a reference-stmt
Minor Issues:
- typedef names should be singular
- access-mode not access-modes
- association-mode not association-modes
- grouping comman-attributes should be common-attributes
- mixed-case naming should not be used (isConfigured)
- indentation used in module needs a lot of corrections
- some descriptions have incorrect tense (e.g., "NTP is enable")
- port definition should be based on inet:port-number, not uint16
OLD:
type uint16 {
range "123 | 1025..max";
}
NEW:
type inet:port-number {
range "123 | 1025..max";
}
- /ntp/authentication/trusted-keys
Why is this list needed? Seems a leaf (e.g., trusted-key (true/false)
added to the authentication-keys list is sufficient
- /ntp/clock-state : some leafs should have a units-stmt instead of
specifying the units in the description-stmt (could also apply elsewhere)
- Examples should use line continuation convention
e.g.:
OLD:
10-10-2017 07:33:55.300 Z+05:30
NEW:
10-10-2017 07:33:55.300 Z+05:30\
- a spell checker should be used; There are many description-stmts
with spelling errors