Subject: [netmod] LL review of 6020bis-06

From: Ladislav Lhotka <[email-masked]>
Date: Mon, 13 Jul 2015 10:52:05 +0200

Hi,

this time I reviewed the complete text so my comments aren't limited to
YANG 1.1 stuff.

Lada

**** General comments
     1. Would it make sense to at least mention that YANG is being
        used not only for NETCONF and with other encodings?
        
     2. I think it would be useful to include text (perhaps in
        sec. 4.2.2) describing the schema tree and data tree, and
        introducing appropriate terms so that, e.g., nodes in either
        tree can be clearly distinguished in the subsequent text. In
        my experience, this causes a lot of confusion.

	For example, sec. 4.2.2.1 says: ' A leaf node contains simple
        data like an integer or a string.' And then sec. 7.6: ' The
        "leaf" statement is used to define a leaf node in the schema
        tree.' If the leaf node is in the schema tree, then it cannot
        contain any value.

     3. It's necessary to clearly separate properties of the data tree
        from properties of XML encoding. For instance, it is an
        inherent property of a container instance in the data tree
        that its children are unordered, or ordered in RPC
        input/output. Yet this is only specified in sec. 7.5.7 (XML
        Mapping Rules).

	Similarly, the namespace of a module and XML namespace that's
        used in XML encoding are IMO two different things (cf. sec
        5.3).
	
     4. Terms "instance"/"instantiate"/"instatiation" are used in a
        loose way without being properly defined, probably as SNMP
        legacy. The XML term "instance document" adds yet another
        meaning.

     5. A term is needed for describing a data model of a particular
        server, i.e. a collection of modules + supported features (+
        deviations). I've used "data model", see
        https://github.com/mbj4668/pyang/wiki/Tutorial#yang-data-model

     6. As Jürgen suggested, sections "XML Mapping Rules" should be
	more appropriately called "XML Encoding Rules".

     7. I'd remove all mentions of submodules including other
        submodules, circular includes etc. because it is now
        confusing. If "include" is ignored in submodules, there is no
        need to bother with any rules. This sentence in sec. 7.1.6
        should be enough: 'For backward compatibility with YANG
        version 1, the "include" statement is permitted in a submodule
        but has no effect there.'

**** Sec. 3
     - Shouldn't the term "data tree" also cover the contents of RPC
       input/output, actions and notifications?

**** Sec. 4.2.2.3
     OLD
         A container may contain any number of child nodes of any
         type (including leafs, lists, containers, and leaf-lists).
     NEW
         A container may contain any number of child nodes of any
         type (leafs, lists, containers, leaf-lists, and actions).

**** Sec. 4.2.9
     - Include an example of action?

**** Sec. 5.2
     - The last paragraph about separate compilation of submodules
       looks like an implementation detail and could perhaps be
       removed.

**** Sec. 5.5
     s/closest matching/matching/

     (because definitions of groupings/typedefs cannot shadow an
      equally named grouping/typedef in an outer scope)

**** Sec. 5.6.4
     - OLD
           A server implements a module if it implements the module's data
           nodes, rpcs, notifications, and deviations.
       NEW
           A server implements a module if it implements the module's data
           nodes, rpcs, actions, notifications, and deviations.
     - s/augement/augment/
     - spurious string '#}}' at the end.

**** Sec. 6.1.3
     I think the rules for whitespace trimming are confusing and have
     very little practical uses, so I'd recommend to remove them
     entirely. However, if the decision is to keep them, the following
     ambiguities need to be addressed:
     - It is unclear whether '\n' is equivalent to the literal LF
       character in the trimming process, i.e. whether the
       substitution of escape sequences is done before or after the
       trimming.
     - Is tab character treated as 8 spaces also before the opening
       double quote? Example:

       description "This is a long
                    text."

       Does it matter whether the character between "description" and
       the opening double quote is a space or tab character? (Both
       cases may have identical visual rendering depending on the
       column where the statement starts.)

**** Sec. 6.2
     OLD
         Implementations MUST support identifiers up to 64 characters in
	 length.
     NEW
         Implementations are not required to support identifier longer
         than 64 character.

     (The current formulation can IMO be understood so that identifiers
     longer than 64 chars cannot be supported.)

**** Sec. 6.3.1
     What does the last paragraph mean? In particular: if an extension
     is imported and used in a module that the server advertises,
     could the server ignore that extension?

**** Sec. 6.4
     - 'The data model used in the XPath expressions is the same as
       that used in XPath 1.0 [XPATH], …'

       I think some explanation is needed describing how a data tree
       is mapped onto the (restricted) XPath data model. Specifically:
       * The data tree has no concept of document order, except for
         RPC input/output. The spec should say implementations need
         to choose some document order but how it is done is
         undefined, so XPath expressions in YANG modules shouldn't
         rely on the document order.
       * It might be useful to define how namespace nodes representing
         YANG namespaces appear in the XPath tree so that the
         namespace:: axis works in a deterministic fashion. For
         example, one could say that all these namespaces are in scope
         for all elements corresponding to YANG data nodes. One could
         use it, e.g. to determine whether a particular YANG module is
         a part of the data model.
     - Another possible source of surprises are equality tests (but
       maybe it belongs to 6087bis). With

       container top {
         must "x = y";
         leaf x {
           type decimal64 {
             fraction-digits 1;
           }
         }
         leaf y {
           type uint8;
         }
       }

       the following instance violates the must constraint:

       <top>
         <x>1.0</x>
         <y>1</y>
       </top>

**** Sec. 6.4.1
     - The accessible tree should also be defined for action
       input/output.
     - Canonical values should be used also for RPCs, actions and
       notifications.
     - s/all all/all/

**** Sec. 7.1.5.1
     - 'The "revision-date" statement MUST match the most recent
       "revision" statement in the imported module.'

       This should be removed because otherwise the importing module
       couldn't stick to an old revision of the imported module.

**** Sec. 7.1.6
     - 'If no "revision-date" substatement is present, it is undefined which
       revision of the submodule is included.'

       I think in this case the submodule revisions need to be
       specified in yang-library.

**** Sec. 7.5.1
     - 'In the first style, the container has no meaning of its own,
       existing only to contain child nodes.'

       I don't think this is really true, e.g. "must" statement on a
       NP containers adds meaning to it, see Y41.

     - 'In the second style, the presence of the container itself is
       configuration data, representing a single bit of configuration
       data.'

       What about containers with presence outside config=true data?

**** Sec. 7.5.7
     - Is the order of subelements also fixed in action input/output?

**** Sec. 7.5.8
     - ' If a container does not have a "presence" statement and the last
       child node is deleted, the NETCONF server MAY delete the
       container.'

       Without further explanation this contradicts the resolution of
       Y41. It should say at least where such containers may be
       deleted (e.g. in the reply to <get-config> or <get>).

**** Sec. 7.6.1
     - The text is suitable for config, defaults in RPC input/output
       and notifications are covered elsewhere, but what about
       defaults (and mandatory) in state data?

**** Sec. 7.7.1
     - The term "array" suggests some higher-level data structure (as
       in JSON encoding). I'd use "sequence of leaf nodes" instead.
     - s/firewall filters/packet filter/

**** Sec. 7.7.4
     - ' The "default" statement MUST NOT be present on nodes where
       "min-elements" has a value greater than or equal to one.'

       Why? This should be OK:

       leaf-list foo {
         type uint8;
         min-elements 1;
         default 54;
       }

**** Sec. 7.7.9
     - Is the interaction of <edit-config> operations, leaf-list
       default values, and with-defaults modes sufficiently clear?
       See also the thread starting with
       https://mailarchive.ietf.org/arch/msg/netmod/PvMzWfzpYa0bg-S2jErX975Y3Jk

**** Sec. 7.8.6
     - It would be helpful to include an example of yang:key attribute
       value with multiple (two) keys. I remember somebody asked me
       about that.

**** Sec. 7.9.2
     - The paragraph about shorthand case statement should more
       explicitly explain that a case node is present in the schema
       tree even for short-case-stmt, and thus schema node identifiers
       always need to include case node identifiers.

**** Sec. 7.10
     - The section should also state that the data model for "anydata"
       content may or may not be known at run time.

**** Sec. 7.12
     - 'For extensions, this means that extensions are applied to the
       grouping node, not the uses node.'

       I dont understand what this means. What is the "grouping node"?

**** Sec. 7.13.2
     - Can a leaf-list also get (new) default values through "refine"?
       If so, how are they combined with the default values defined in
       the grouping?

**** Sec. 7.15
     - 'An action MUST NOT be defined within an rpc, another action or a
        notification, …'

       Also it MUST NOT be defined within leaf and leaf-list, right?

     - Can an action defined on a list be applied to the whole list?

**** Sec. 7.21.3
     - Maybe this section should also explain that descriptions are an
       integral part of the data model and may specify, e.g., semantic
       constraints that cannot be expressed formally.

**** Sec. 9.10.2
     - s/supported by the server/implemented by the server/

**** Sec. 9.10.3
     The last paragraph should probably be replaced with an advice that
     identityref values or nodesets should never appear as bare
     strings in XPath expressions, and functions derived-from() or
     derived-from-or-self() should be used instead. Example in
     sec. 9.10.5 should be changed to use derived-from-or-self().

**** Sec. 10.3.1
     - The document order is undefined in a data tree (also in
       sec. 10.4.1 and 10.4.2).
     - I think deref() could return non-empty nodeset only for
       instance-identifiers because for leafref it's not needed. The
       "must" constraint in sec. 10.3.1.1 is better expressed like so:
       '/interface[name=current()]/enabled = "true"'

**** Sec. 13
     - The ABNF can be made stricter by replacing "[default-stmt]"
       among the substatements of "choice-stmt" with
       "[default-case-stmt]", and then adding

       default-case-stmt = default-keyword sep node-identifier stmtend

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: E74E8C0C