Subject: [yang-doctors] Yangdoctors early review of draft-ietf-bfd-unsolicited-01

From: Martin Björklund via Datatracker <[email-masked]>
Date: Mon, 19 Aug 2019 04:29:09 -0700

Reviewer: Martin Björklund
Review result: Ready with Nits

I have reviewed this document from a YANG model perspective only.

My only comment is actually for a grouping defined in ietf-bfd-type, but used
in this module.  There is a choice "interval-config-type":

  +--rw unsolicited {bfd-unsol:unsolicited-params-global}?
       +--rw enable?                           boolean
       +--rw local-multiplier?                 multiplier
       +--rw (interval-config-type)?
          +--:(tx-rx-intervals)
          |  +--rw desired-min-tx-interval?    uint32
          |  +--rw required-min-rx-interval?   uint32
          +--:(single-interval) {single-minimum-interval}?
             +--rw min-interval?               uint32

This choice is not mandatory and doesn't have a default case, so the question
is what happens if no nodes from the choice has been configured?   I would
expect the choice to have a default case (but this then would apply to
ietf-bfd-types, not this document.)

---

From: Mahesh Jethanandani <[email-masked]>
Date: Mon, 19 Aug 2019 11:45:05 -0700

[Adding the authors of BFD YANG module]

Martin brings up a good point. But since the document that contains ietf-bfd-types is sitting in RFC Ed Queue, this will have to go into a bis document.

Chairs, could you create a bfd-wg in GitHub for us to track this as an issue to be fixed as part of a bis document?

> On Aug 19, 2019, at 4:29 AM, Martin Björklund via Datatracker <[email-masked]> wrote:
> 

Mahesh Jethanandani
[email-masked]

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Mon, 19 Aug 2019 19:11:07 +0000

Thanks Martin and Mahesh.

I believe we should add a mandatory statement to the choic (speaking as BFD YANG co-author,)

Just created https://github.com/bfd-wg

Regards,
Reshad.

On 2019-08-19, 2:45 PM, "Mahesh Jethanandani" <[email-masked]> wrote:

    

    Mahesh Jethanandani
    [email-masked]

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 19 Aug 2019 21:18:37 +0200 (CEST)

> I believe we should add a mandatory statement to the choic (speaking

But then it is not clear why all leafs in the cases have default
statements.

Since the 'single-interval' case is optional with a if-feature (which
BTW is weird since it is trivial to implement), and the only other
case has default values on both its leafs, I would have assumed that
the intention was that if nothing is configured, the server should use
1000000 microseconds for the intervals.  If this is the intention,
perhaps a statement:  "default tx-rx-intervals;" can be added to the
module, even though the doc is in the RFC ed q.

/martin

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Mon, 19 Aug 2019 19:31:27 +0000

I was looking at an old copy of the doc which didn't have default. So yes, mandatory doesn't make sense with the default statements.

Your assumption below wrt the intention is correct. I don't know how feasible it is to add this while it's in the editor q.

Regards,
Reshad.

On 2019-08-19, 3:18 PM, "Martin Bjorklund" <[email-masked]> wrote:

    

    /martin

---

From: Jeffrey Haas <[email-masked]>
Date: Wed, 21 Aug 2019 16:13:28 -0400

Reshad,

If procedures permit it (I'm unclear on the detail), does it make sense to
pull the BFD yang module for a fix from the editor queue?

-- Jeff

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Wed, 21 Aug 2019 20:13:59 +0000

Hi Jeff,

Yes, to me it makes sense to do the change suggested by Martin (add "default tx-rx-intervals;" to the choice statement). BFD YANG co-authors, please respond asap if you disagree.

Regards,
Reshad.

On 2019-08-21, 4:11 PM, "Jeffrey Haas" <[email-masked]> wrote:

    Reshad,

    -- Jeff

---

From: "Reshad Rahman (rrahman)" <[email-masked]>
Date: Wed, 21 Aug 2019 22:33:01 +0000

And this is what the changes would look like.

On 2019-08-21, 4:14 PM, "Reshad Rahman (rrahman)" <[email-masked]> wrote:

    Hi Jeff,

    Regards,
    Reshad.

        Reshad,

        -- Jeff

---

From: Jeffrey Haas <[email-masked]>
Date: Fri, 23 Aug 2019 10:35:39 -0400

> And this is what the changes would look like.

That looks fine to me.  I don't expect it to be controversial.

-- Jeff

> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
> <!-- saved from url=(0049)https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht -->
> <html xmlns="http://www.w3.org/1999/xhtml"><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> 
>    
>   <meta http-equiv="Content-Style-Type" content="text/css"> 
>   <title>Diff: draft-ietf-bfd-yang.txt - draft-ietf-bfd-yang-17.txt</title> 
>   <style type="text/css"> 
>     body    { margin: 0.4ex; margin-right: auto; } 
>     tr      { } 
>     td      { white-space: pre; font-family: monospace; vertical-align: top; font-size: 0.86em;} 
>     th      { font-size: 0.86em; } 
>     .small  { font-size: 0.6em; font-style: italic; font-family: Verdana, Helvetica, sans-serif; } 
>     .left   { background-color: #EEE; } 
>     .right  { background-color: #FFF; } 
>     .diff   { background-color: #CCF; } 
>     .lblock { background-color: #BFB; } 
>     .rblock { background-color: #FF8; } 
>     .insert { background-color: #8FF; } 
>     .delete { background-color: #ACF; } 
>     .void   { background-color: #FFB; } 
>     .cont   { background-color: #EEE; } 
>     .linebr { background-color: #AAA; } 
>     .lineno { color: red; background-color: #FFF; font-size: 0.7em; text-align: right; padding: 0 2px; } 
>     .elipsis{ background-color: #AAA; } 
>     .left .cont { background-color: #DDD; } 
>     .right .cont { background-color: #EEE; } 
>     .lblock .cont { background-color: #9D9; } 
>     .rblock .cont { background-color: #DD6; } 
>     .insert .cont { background-color: #0DD; } 
>     .delete .cont { background-color: #8AD; } 
>     .stats, .stats td, .stats th { background-color: #EEE; padding: 2px 0; } 
>     span.hide { display: none; color: #aaa;}    a:hover span { display: inline; }    tr.change { background-color: gray; } 
>     tr.change a { text-decoration: none; color: black } 
>   </style> 
>      <script>
> var chunk_index = 0;
> var old_chunk = null;
> 
> function format_chunk(index) {
>     var prefix = "diff";
>     var str = index.toString();
>     for (x=0; x<(4-str.length); ++x) {
>         prefix+='0';
>     }
>     return prefix + str;
> }
> 
> function find_chunk(n){
>     return document.querySelector('tr[id$="' + n + '"]');
> }
> 
> function change_chunk(offset) {
>     var index = chunk_index + offset;
>     var new_str;
>     var new_chunk;
> 
>     new_str = format_chunk(index);
>     new_chunk = find_chunk(new_str);
>     if (!new_chunk) {
>         return;
>     }
>     if (old_chunk) {
>         old_chunk.style.outline = "";
>     }
>     old_chunk = new_chunk;
>     old_chunk.style.outline = "1px solid red";
>     window.location.replace("#" + new_str)
>     window.scrollBy(0,-100);
>     chunk_index = index;
> }
> 
> document.onkeydown = function(e) {
>     switch (e.keyCode) {
>     case 78:
>         change_chunk(1);
>         break;
>     case 80:
>         change_chunk(-1);
>         break;
>     }
> };
>    </script> 
> </head> 
> <body> 
>   <table border="0" cellpadding="0" cellspacing="0"> 
>   <tbody><tr id="part-1" bgcolor="orange"><th></th><th><a href="https://tools.ietf.org/rfcdiff?url2=draft-ietf-bfd-yang.txt" style="color:#008; text-decoration:none;">&lt;</a>&nbsp;<a href="https://tools.ietf.org/html/draft-ietf-bfd-yang.txt" style="color:#008">draft-ietf-bfd-yang.txt</a>&nbsp;</th><th> </th><th>&nbsp;<a href="https://tools.ietf.org/html/draft-ietf-bfd-yang-17.txt" style="color:#008">draft-ietf-bfd-yang-17.txt</a>&nbsp;<a href="https://tools.ietf.org/rfcdiff?url1=draft-ietf-bfd-yang-17.txt" style="color:#008; text-decoration:none;">&gt;</a></th><th></th></tr> 
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr id="part-1" class="change"><td></td><th><small>skipping to change at</small><a href="https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht#part-1"><em> page 26, line 26<span class="hide"> ??</span></em></a></th><th> </th><th><small>skipping to change at</small><a href="https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht#part-1"><em> page 26, line 26<span class="hide"> ??</span></em></a></th><td></td></tr>
>       <tr><td class="lineno"></td><td class="left">    }</td><td> </td><td class="right">    }</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  }</td><td> </td><td class="right">  }</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  &lt;CODE ENDS&gt;</td><td> </td><td class="right">  &lt;CODE ENDS&gt;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">2.13.  BFD types YANG Module</td><td> </td><td class="right">2.13.  BFD types YANG Module</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">   This YANG module imports typedefs from [RFC6991], [RFC8177] and the</td><td> </td><td class="right">   This YANG module imports typedefs from [RFC6991], [RFC8177] and the</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">   "control-plane-protocol" identity from [RFC8349].</td><td> </td><td class="right">   "control-plane-protocol" identity from [RFC8349].</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr id="diff0001"><td></td></tr>
>       <tr><td class="lineno"></td><td class="lblock">&lt;CODE BEGINS&gt; file "ietf-bfd-types@201<span class="delete">8-08-0</span>1.yang"</td><td> </td><td class="rblock">&lt;CODE BEGINS&gt; file "ietf-bfd-types@201<span class="insert">9-08-2</span>1.yang"</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">module ietf-bfd-types {</td><td> </td><td class="right">module ietf-bfd-types {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  yang-version 1.1;</td><td> </td><td class="right">  yang-version 1.1;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  namespace "urn:ietf:params:xml:ns:yang:ietf-bfd-types";</td><td> </td><td class="right">  namespace "urn:ietf:params:xml:ns:yang:ietf-bfd-types";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  prefix "bfd-types";</td><td> </td><td class="right">  prefix "bfd-types";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  // RFC Ed.: replace occurences of XXXX with actual RFC number and</td><td> </td><td class="right">  // RFC Ed.: replace occurences of XXXX with actual RFC number and</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr id="part-2" class="change"><td></td><th><small>skipping to change at</small><a href="https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht#part-2"><em> page 27, line 49<span class="hide"> ??</span></em></a></th><th> </th><th><small>skipping to change at</small><a href="https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht#part-2"><em> page 27, line 49<span class="hide"> ??</span></em></a></th><td></td></tr>
>       <tr><td class="lineno"></td><td class="left">     to the license terms contained in, the Simplified BSD License</td><td> </td><td class="right">     to the license terms contained in, the Simplified BSD License</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">     set forth in Section 4.c of the IETF Trust's Legal Provisions</td><td> </td><td class="right">     set forth in Section 4.c of the IETF Trust's Legal Provisions</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">     Relating to IETF Documents</td><td> </td><td class="right">     Relating to IETF Documents</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">     (http://trustee.ietf.org/license-info).</td><td> </td><td class="right">     (http://trustee.ietf.org/license-info).</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">     This version of this YANG module is part of RFC XXXX; see</td><td> </td><td class="right">     This version of this YANG module is part of RFC XXXX; see</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">     the RFC itself for full legal notices.";</td><td> </td><td class="right">     the RFC itself for full legal notices.";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  reference "RFC XXXX";</td><td> </td><td class="right">  reference "RFC XXXX";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr id="diff0002"><td></td></tr>
>       <tr><td class="lineno"></td><td class="lblock">  revision 201<span class="delete">8-08-0</span>1 {</td><td> </td><td class="rblock">  revision 201<span class="insert">9-08-2</span>1 {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    description "Initial revision.";</td><td> </td><td class="right">    description "Initial revision.";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    reference "RFC XXXX: YANG Data Model for BFD";</td><td> </td><td class="right">    reference "RFC XXXX: YANG Data Model for BFD";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  }</td><td> </td><td class="right">  }</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  /*</td><td> </td><td class="right">  /*</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">   * Feature definitions</td><td> </td><td class="right">   * Feature definitions</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">   */</td><td> </td><td class="right">   */</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  feature single-minimum-interval {</td><td> </td><td class="right">  feature single-minimum-interval {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    description</td><td> </td><td class="right">    description</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">      "This feature indicates that the server supports configuration</td><td> </td><td class="right">      "This feature indicates that the server supports configuration</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">       of one minimum interval value which is used for both transmit and</td><td> </td><td class="right">       of one minimum interval value which is used for both transmit and</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr id="part-3" class="change"><td></td><th><small>skipping to change at</small><a href="https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht#part-3"><em> page 31, line 43<span class="hide"> ??</span></em></a></th><th> </th><th><small>skipping to change at</small><a href="https://tools.ietf.org/tools/rfcdiff/rfcdiff.pyht#part-3"><em> page 31, line 43<span class="hide"> ??</span></em></a></th><td></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">  grouping base-cfg-parms {</td><td> </td><td class="right">  grouping base-cfg-parms {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    description "BFD grouping for base config parameters.";</td><td> </td><td class="right">    description "BFD grouping for base config parameters.";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    leaf local-multiplier {</td><td> </td><td class="right">    leaf local-multiplier {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">      type multiplier;</td><td> </td><td class="right">      type multiplier;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">      default 3;</td><td> </td><td class="right">      default 3;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">      description "Multiplier transmitted by local system.";</td><td> </td><td class="right">      description "Multiplier transmitted by local system.";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    }</td><td> </td><td class="right">    }</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left"></td><td> </td><td class="right"></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">    choice interval-config-type {</td><td> </td><td class="right">    choice interval-config-type {</td><td class="lineno"></td></tr>
>       <tr id="diff0003"><td></td></tr>
>       <tr><td class="lineno"></td><td class="lblock"></td><td> </td><td class="rblock"><span class="insert">      default tx-rx-intervals;</span></td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">      description</td><td> </td><td class="right">      description</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">        "Two interval values or one value used for both transmit and</td><td> </td><td class="right">        "Two interval values or one value used for both transmit and</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">         receive.";</td><td> </td><td class="right">         receive.";</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">      case tx-rx-intervals {</td><td> </td><td class="right">      case tx-rx-intervals {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">        leaf desired-min-tx-interval {</td><td> </td><td class="right">        leaf desired-min-tx-interval {</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">          type uint32;</td><td> </td><td class="right">          type uint32;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">          units microseconds;</td><td> </td><td class="right">          units microseconds;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">          default 1000000;</td><td> </td><td class="right">          default 1000000;</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">          description</td><td> </td><td class="right">          description</td><td class="lineno"></td></tr>
>       <tr><td class="lineno"></td><td class="left">            "Desired minimum transmit interval of control packets.";</td><td> </td><td class="right">            "Desired minimum transmit interval of control packets.";</td><td class="lineno"></td></tr>
> 
>      <tr><td></td><td class="left"></td><td> </td><td class="right"></td><td></td></tr>
>      <tr id="end" bgcolor="gray"><th colspan="5" align="center">&nbsp;End of changes. 3 change blocks.&nbsp;</th></tr>
>      <tr class="stats"><td></td><th><i>2 lines changed or deleted</i></th><th><i> </i></th><th><i>3 lines changed or added</i></th><td></td></tr>
>      <tr><td colspan="5" align="center" class="small"><br>This html diff was produced by rfcdiff 1.47. The latest version is available from <a href="http://www.tools.ietf.org/tools/rfcdiff/">http://tools.ietf.org/tools/rfcdiff/</a> </td></tr>
>    </tbody></table>
>    
>    
> </body></html>