Re: [Softwires] Yangdoctors last call review of draft-ietf-softwire-yang-06
<mohamed.boucadair@orange.com> Tue, 23 October 2018 05:59 UTC
Return-Path: <mohamed.boucadair@orange.com>
X-Original-To: softwires@ietfa.amsl.com
Delivered-To: softwires@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 911ED130E89; Mon, 22 Oct 2018 22:59:58 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.599
X-Spam-Level:
X-Spam-Status: No, score=-2.599 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, RCVD_IN_DNSWL_LOW=-0.7, SPF_PASS=-0.001, UNPARSEABLE_RELAY=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0ey0xitbQXXC; Mon, 22 Oct 2018 22:59:56 -0700 (PDT)
Received: from orange.com (mta136.mail.business.static.orange.com [80.12.70.36]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 1798E130E90; Mon, 22 Oct 2018 22:59:56 -0700 (PDT)
Received: from opfednr01.francetelecom.fr (unknown [xx.xx.xx.65]) by opfednr26.francetelecom.fr (ESMTP service) with ESMTP id 42fN5Q46rqz10Vh; Tue, 23 Oct 2018 07:59:54 +0200 (CEST)
Received: from Exchangemail-eme2.itn.ftgroup (unknown [xx.xx.31.19]) by opfednr01.francetelecom.fr (ESMTP service) with ESMTP id 42fN5Q3BvwzDq78; Tue, 23 Oct 2018 07:59:54 +0200 (CEST)
Received: from OPEXCLILMA3.corporate.adroot.infra.ftgroup ([fe80::60a9:abc3:86e6:2541]) by OPEXCLILM44.corporate.adroot.infra.ftgroup ([fe80::b08d:5b75:e92c:a45f%18]) with mapi id 14.03.0415.000; Tue, 23 Oct 2018 07:59:54 +0200
From: mohamed.boucadair@orange.com
To: Martin Björklund <mbj@tail-f.com>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "softwires@ietf.org" <softwires@ietf.org>, "draft-ietf-softwire-yang.all@ietf.org" <draft-ietf-softwire-yang.all@ietf.org>, "ietf@ietf.org" <ietf@ietf.org>
Thread-Topic: Yangdoctors last call review of draft-ietf-softwire-yang-06
Thread-Index: AQHUZGV23P0mXi3OdECuF60fHv3ryqUsV0rQ
Date: Tue, 23 Oct 2018 05:59:53 +0000
Message-ID: <787AE7BB302AE849A7480A190F8B93302E0198E5@OPEXCLILMA3.corporate.adroot.infra.ftgroup>
References: <153959399912.4621.9236297320162816916@ietfa.amsl.com>
In-Reply-To: <153959399912.4621.9236297320162816916@ietfa.amsl.com>
Accept-Language: fr-FR, en-US
Content-Language: fr-FR
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-originating-ip: [10.168.234.4]
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/softwires/emmtVsHeGjnE9hDgKiBEJ0ocXB4>
Subject: Re: [Softwires] Yangdoctors last call review of draft-ietf-softwire-yang-06
X-BeenThere: softwires@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: softwires wg discussion list <softwires.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/softwires>, <mailto:softwires-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/softwires/>
List-Post: <mailto:softwires@ietf.org>
List-Help: <mailto:softwires-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/softwires>, <mailto:softwires-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 23 Oct 2018 05:59:59 -0000
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@tail-f.com] > Envoyé : lundi 15 octobre 2018 11:00 > À : yang-doctors@ietf.org > Cc : softwires@ietf.org; draft-ietf-softwire-yang.all@ietf.org; ietf@ietf.org > 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 >
- [Softwires] Yangdoctors last call review of draft… Martin Björklund
- Re: [Softwires] Yangdoctors last call review of d… mohamed.boucadair
- Re: [Softwires] Yangdoctors last call review of d… Martin Bjorklund
- Re: [Softwires] Yangdoctors last call review of d… mohamed.boucadair
- Re: [Softwires] Yangdoctors last call review of d… Martin Bjorklund
- Re: [Softwires] Yangdoctors last call review of d… mohamed.boucadair
- Re: [Softwires] Yangdoctors last call review of d… Martin Bjorklund
- Re: [Softwires] Yangdoctors last call review of d… mohamed.boucadair
- Re: [Softwires] Yangdoctors last call review of d… tom petch
- Re: [Softwires] Yangdoctors last call review of d… mohamed.boucadair