[netmod] js review of draft-ietf-core-sid-12

[Juergen Schoenwaelder <[e-mail]>]

Hi,

I have reviewed draft-ietf-core-sid-12. I will try to review yang-cbor
tomorrow as well. (I am probably more interested in CBOR encoding for
RESTCONF than the usage of CoAP itself, hence my priority on these two
IDs under last call.)

- Avoid the use of the word 'template', which is not a well defined
  term and may be used with varying interpretations.

   o  data nodes (Note: including those parts of a YANG template as
      defined by the 'yang-data' extension.)

  It is not clear to me what is meant with "those parts of a YANG
  template as defined by the 'yang-data' extension." I think this
  could benefit from a rewording.

- The ID seems to assume that semantics of yang items never change.
  This is true so far but NETMOD has chartered work that might change
  this property. So what happens if the semantics of a YANG item
  changes?

   SIDs are assigned permanently, items introduced by a new revision of
   a YANG module are added to the list of SIDs already assigned.

  If a YANG module changes in a non-backwards compatible way, I assume
  a new sid range must be allocated? Strictly speaking, this question
  does not have to be answered today but it very likely needs an
  answer in the future...

- The definition of 'path' is not very precise, i.e., it does not
  spell out how module namespaces work, only by example.. I do not
  know whether a definition can be imported from somewhere.

- s/RESCONF/RESTCONF/

- Is it CoRECONF or CORECONF? And I find the term CORECONF confusing.
  We have two protocols called NETCONF and RESTCONF and now we add
  another protocol called CoMI and we call CoMI together with YANG
  CBOR and SIDs CORECONF?

  1) NETCONF  + YANG + XML      serialization + path naming -> ?
  2) RESTCONF + YANG + XML|JSON serialization + path naming -> ?
  3) CoMI     + YANG + CBOR     serialization + SID naming  -> CORECONF

  We do not have a term for 1) and 2) and then we have a term for 3)
  which, however, looks more like the protocol names used in 1) and
  2). This comment is not specific to this ID, but the asymmetry
  showed up while reading the SID document, I had to look at other IDs
  to understand how things are named. And the SID document says

   YANG is a language designed to model data accessed using one of the
   compatible protocols (e.g.  NETCONF [RFC6241], RESCONF [RFC8040] and
   CoRECONF [I-D.ietf-core-comi]).

  Then I read the CoMI abstract. It first says CoMI is "a CoAP
  Management Interface", it then says "The complete solution composed
  of CoMI, [I-D.ietf-core-yang-cbor] and [I-D.ietf-core-sid] is called
  CORECONF." and finally it states that "CORECONF extends the set of
  YANG based protocols, NETCONF and RESTCONF, with the capability to
  manage constrained devices and networks.". So I am confused, is
  CORECONF a protocol as stated in this document? Or is CoMI a
  protocol? (What is then the difference between a "Management
  Interface" and a management protocol?) I am not sure whether I get
  to review comi, hence I mention my confusion here as I hit it while
  reviewing the sid document.

- This description makes little sense to me:

  typedef sid-file-version-identifier {
    type uint64;
    description
      "Optional attribute that gives information about the .sid file
       version.";
  }

  This is a type definition. Why does the description talk about an
  optional attribute? The type should not state whether something
  using the type is optional or not. (And I would prefer to avoid
  'attribute', better use YANG defined terms or just describe that
  this type represents a version number for a SID file.)

- sid range - was it not said before that it is 63 bits? Is the idea
  that sids with the highest bit set are legal but undefined or
  reserved?  Or should there be a range restriction?

  typedef sid {
    type uint64;
    description
      "YANG Schema Item iDentifier";
    reference
      "[I-D.ietf-core-sid] YANG Schema Item iDentifier (SID)";
  }

- schema-node-path

      "Identifies a schema-node path string for use in the
       SID registry. This string format follows the rules
       for an instance-identifier, as defined in RFC 7959,
       except that no predicates are allowed.

  RFC 7959 seems to be a typo, I assume you mean RFC 7951

  s/Identifies a schema-node path string/A schema-node path"

  It is a bit confusing to define a schema-node path by way of
  reference to an instance identifier. I understand that you borrow
  the namespace encoding from the way JSON encode instance identifiers
  but this type really represents what RFC 7950 calls an absolute
  schema node identifier, no? Is the term schema-node path actually
  needed or is it the same as absolute schema node identifier? Or is
  the difference between the two how namespaces are represented?

- dependency-revision

        list dependency-revision {
	key "module-name";

	description
          "Information about the revision of each YANG module that the
	  module in
          'module-name' includes used during the .sid file generation.";

   The sentence can probably be rewritten to make it clearer.

   Are the SIDs assigned to a module dependent on the modules listed
   in the 'dependency-revision'? (Is this a good name for the list?)
   Why is it necessary to know which revisions were used to resolve
   imports without revision?

- Please avoid wrapped entry point numbers in the table in 6.3.3.

- The handling of early allocations sounds complex, I have some doubts
  this process will work in practice...

- Incomplete sentence:

  RFC7120] also says

  Or is the following paragraph a quote? In that case, add a colon.

- There are likely more normative references, e.g., RFC 6991 and RFC
  8040.

- Why does the example in appendix A not have / need
  dependency-revisions?

- I have not run any tools to validate things. I just read the I-D.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: [phone]         Campus Ring 1 | 28759 Bremen | Germany
Fax:   [phone]         <https://www.jacobs-university.de/>