Hi, <mohamed.boucadair@xxxxxxxxxx> wrote: > 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. Thank's for addressing my comments. See inline and at the end for some new comments on -08. > 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. I think that the addition of identities for various tunnel types that derive from ift:tunnel is the right thing to do. However, since these identities derive from ift:tunnel, the augmentation of ietf-interfaces in ietf-interface-tunnel is not needed. Instead, the new identities should be used as if:type directly. For example, instead of: <interface xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type"> <name>lw4o6-wan</name> <type>ianaift:tunnel</type> <tunnel-type xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-type"> iana-tunnel-type:aplusp </tunnel-type> ... </interface> you should do: <interface> xmlns:iana-tunnel-type="urn:ietf:params:xml:ns:yang:iana-tunnel-type"> <name>lw4o6-wan</name> <type>iana-tunnel-type:aplusp</type> ... </interface> So, I think you should remove the ietf-tunnel-type module. An additional comment on the identities in iana-tunnel-type; for each identity, I think you should add a "reference" statement that points to the RFC(s) that define the tunnel. (available in the IANA registry at https://www.iana.org/assignments/smi-numbers/smi-numbers.xhtml#smi-numbers-5) > > 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. But you have for example in the description of "binding-mode": This feature indicates that the instance functions as a binding based softwire instance. And in container algo-instances you have: The instances advertise the MAP-E/MAP-T feature through the capability exchange mechanism when a NETCONF session is established." Unless your intentation is that one "instance" == one "function" == one NETCONF server, then this text is not correct. So I am a bit confused - if the device advertises the feature "binding-mode" it means that "it functions as a binding based softwire instance". Maybe you mean something along the lines of This feature indicates that the network element can function as one or more binding based softwire instances. (I don't know if you want to use the term "network element" or something else.) Also there is similar text for the features map-e and map-t. Anyway, if this meaning of the word "instance" is defined somewhere, I suggest you add a reference to that other doc; or else explain this meaning in 1.1. > 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. I could guess that. I think the issue is when the word "instance" is used unqualified. > > 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. This text is still present, with a minor change: container algo-instances { description "Indicates that the instances supports the MAP-E and/or MAP-T function. The instances advertise the MAP-E/MAP-T feature through the capability exchange mechanism when a NETCONF session is established."; But since the container "algo-instances" is a non-presence container, it can't "indicate" anything. This text needs to be rephrased. > > 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. Ok. I still don't really understand how it is supposed to be used. When you write "version number for the algorithm", do you mean "version number for this algo-instance"? These are config true leafs; should they be config false and internally managed by the server? If not, I suppose an operator can set them to any suitable values? If so, what does "incremental version number" really mean? Is the server supposed to reject a value that is not "incremental"? [...] > > 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. Well, in MIBs instances are normally identified with integers b/c of how the protocol (SNMP) works. In YANG modules, we usually use a "name" that is a string to identify instances. With a string, the operator can choose meaningful names, and use them in other leafrefs, instead of having to remember how the names are mapped to integers. (Compare ifIndex (MIB) w/ interface/name (YANG)) So I suggest you use "name" as key. o I also note that you have changed the name of some lists in -08, e.g., list "bind-instance" is now list "bind-instances" (plural). Another example is: +--rw algo-instances +--rw algo-instances* [id] I think you should change these back to singluar; that's what YANG modules typically use. /martin
