Reviewer: Martin Björklund Review result: Ready with Issues This is my YANG doctor review of draft-ietf-softwire-yang-06. The review focuses on YANG-specifics only, since I am not familiar with the technology that is modelled. o I would like to see all Tom Petch's comments in his three replies to the IETF LC for this document addressed. Especially his comment about using ianatf:tunnel. o All three YANG modules should follow the common template for IETF YANG modules found in https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis. Specifically, fix the authors list and copyright text. o The term "instance" is used to mean - I think - the "device". I didn't find this term in the RFCs 7596, 7597 or 7599. I suggest you use some other term, since "instance" is quite generic, and is often used to refer to instances of YANG data nodes. Also, does the term "instance" mean the same thing in "algo-instance"? And in "br-instances"? "bind-instance"? o In ietf-softwire-common: grouping algorithm-instance { description "Indicates that the instance supports the MAP-E and MAP-T function. The instance advertises the MAP-E/MAP-T feature through the capability exchange mechanism when a NETCONF session is established."; This description does not seem right. A grouping can't indicate anything. Also, what is "the MAP-E/MAP-T feature"? o In ietf-softwire-ce: A similar description is found here: container algo-instances { description "Indicates that the instances supports the MAP-E and MAP-T function. The instances advertise the MAP-E/MAP-T feature through the capability exchange mechanism when a NETCONF session is established."; same comments apply. o In ietf-softwire-common: container algo-versioning { description "algorithm's version"; leaf version { type uint64; description "Incremental version number for the algorithm"; } leaf date { type yang:date-and-time; description "Timestamp to the algorithm"; } } Maybe these descriptions are crystal clear to someone who knows the technology. If so, perhaps add a reference to help the rest of us? o In general, it would help if the definitions had "reference" statements to the relevant sections in the underlying RFCs. o In ietf-softwire-ce: notification softwire-ce-event { if-feature binding; description "CE notification"; leaf ce-binding-ipv6-addr-change { type inet:ipv6-address; mandatory true; description "If the CE's binding IPv6 address changes for any reason, it should notify the NETCONF client."; } Perhaps rewrite the description to: This notification is generated whenever the CE's binding IPv6 address changes for any reason. o In ietf-softwire-br: You have: leaf softwire-num-max leaf softwires-payload-mtu leaf softwire-path-mru Any reason it isn't "softwire-payload-mtu"? o In ietf-softwire-br: list bind-instance { key "id"; [...] container binding-table-versioning { ... } leaf id { ... } I suggest you put the leaf "id" before the container; it is common practise to put the key leafs first. Also, the leaf id has the description: description "An instance identifier."; This again can be confused with YANG's "instance-identifier". Also, it seems each "instance" has a numerical id (the key), but also a name (a string). Maybe there are protocol-reasons to do this, but if not, did you consider using the "name" as key instead? o In ietf-softwire-br: notification softwire-binding-instance-event { if-feature binding; description "Notifications for binding instance."; notification softwire-algorithm-instance-event { if-feature algorithm; description "Notifications for algorithmic instance."; I suggest that the descriptions are updated to describe when these notifications are generated. o Appendix A The examples have: <softwire-config xmlns="urn:ietf:params:xml:ns:yang:ietf-softwire-br"> which isn't present in the module. Maybe a leftover? o Formatting To save the RFC editor some cycles, I suggest you format the modules consistently wrt. whitespaces/newlines and indentation. One option is to run: pyang -f yang --keep-comments <filename> and then manually fix the long leafref paths that pyang can't handle properly at the moment. /martin
