[Softwires] Yangdoctors last call review of draft-ietf-softwire-yang-06

[Martin Björklund <[e-mail]>]

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