Hi Martin, Thank you for the review. We released a new revision -08 to address your comments. We will double check the formatting and issue another iteration if needed. Please see inline. Cheers, Med > -----Message d'origine----- > De : Martin Björklund [mailto:mbj@xxxxxxxxxx] > Envoyé : lundi 15 octobre 2018 11:00 > À : yang-doctors@xxxxxxxx > Cc : softwires@xxxxxxxx; draft-ietf-softwire-yang.all@xxxxxxxx; ietf@xxxxxxxx > Objet : Yangdoctors last call review of draft-ietf-softwire-yang-06 > > 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. > [Med] This is fixed in -07. A new tunnel-type parameter is defined to handle this. > > 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. > [Med] Done. > > o The term "instance" is used to mean - I think - the "device". [Med] It is used to mean function rather than device. A device may enable multiple instances of the same function. 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"? > > [Med] algo-instance means an instance of type algorithm. br-instances denotes a set of br instances, and bind-instance means an instance of type binding. > > 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"? > [Med] Fixed. > > 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. > [Med] Fixed. > > 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? > [Med] This is used for logging purposes. A reference to RFC7422 is added. > > 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. > [Med] OK. > > 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"? > [Med] No. Fixed already in -07. > > 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. > [Med] Fixed. > Also, the leaf id has the description: > > description "An instance identifier."; > > This again can be confused with YANG's "instance-identifier". > [Med] chnaged to: "A binding instance identifier. This identifier can be automatically assigned or explicitly configured."; > 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? > [Med] id/name is inspired from how NAT instances are managed (see RFC7659). The name is optional. > > 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. > [Med] Done. > > 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? > [Med] Ooops. We used to have that before moving to an nmda-compliant module. > > 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. > [Med] Will double check this one. > > /martin >
