Subject: [netmod] LL review of draft-ietf-netmod-rfc6020bis-02

From: Ladislav Lhotka <[email-masked]>
Date: Wed, 26 Nov 2014 20:25:05 +0100

Hi,

below are my comments to YANG 1.1 draft.

Lada

**** Sec. 4.2.7
     I think this is incorrect terminology: "When a node from one case
     is created, ..." - sec. 3 defines "data node" as a node in the
     schema tree. It should be "When a data node instance from one
     case is created, ...". Sec. 3 should probably also define what
     "instance" means.
**** Sec. 4.2.2.2
     OLD
         A leaf-list is a sequence of leaf nodes with exactly one value of a
	 particular type per leaf.
     NEW
         A leaf-list instance is a sequence of values of a particular type.
**** Sec. 7.5.3
     The issue Y41 is marked as REVIEW, so I'd expect some text in
     this section explaining how "must" expressions on NP-containers
     are evaluated.
**** Sec. 7.19.5
     The second paragraph should say that a list key MUST NOT have a
     "when" statement. Such a statement has a different context node
     than a "when" statement on its ancestor, so it could break even
     if the two statements are identical.
**** Sec. 9.10.3
     In the last paragraph, what is "the prefix"? It depends on
     namespace declarations that are in effect, and there can even be
     multiple prefixes declared for the same URI.

     Therefore, I think the string value has to be the exact value of
     the node (with no prefix if there is none). Consequently,
     comparison of identities in XPath expressions should always be
     performed via special XPath functions.
**** Sec. 9.10.5
     The "when" expression in the example should be

     when 'derived-from-or-self(../crypto, "my-crypto", "aes")';

     (see also my comment to sec. 10.4.1 below).
**** Sec. 9.12
     The validation procedure is too XML-centric. The value of a union
     instance needn't be a string.

     OLD
         When a string representing a union data type is validated,
         the string is validated against each member type, ...
     NEW
         A value representing a union data type is validated
         consecutively against each member type, ...
**** Sec. 10
     - The first paragraph should say that function signatures
       are specified in the same form as in XPath 1.0 spec.
     - Titles of subsections 10.1 and 10.2 could be "Node Set
       Functions" and "String Functions" (XPath spec uses these
       titles).
***** Sec. 10.1
      OLD
          The function current() takes no input parameters, and
          returns a node set with the initial context node.
      NEW
          The function current() takes no input parameters, and
          returns a node-set that has the context node as its only
          member.
***** Sec. 10.3.1
      Instead of the deref() function I'd suggest to define a more
      general function evaluate(), that already exists in EXSLT, see
      http://www.exslt.org/dyn/functions/evaluate/:

      object evaluate(string expr)

      The evaluate() function takes a string, evaluates it as an XPath
      expression and returns the resulting value, which may be a
      node-set, string, number or boolean.
***** Sec. 10.4.1
      A similar function that's also needed is derived-from-or-self().
      It would be needed in example 10.4.1.1 for doing things like

      augment "/interface" {
          when 'derived-from-or-self(type,
	                     "example-interface",
			     "fast-ethernet")';
          // fast-ethernet-specific definitions here
      }

      Alternatively, we could state in YANG 1.1 that derivation of
      identities is a reflexive relation, and then derived-from()
      would probably suffice.
***** Sec. 10.6.1
      Maybe bit() is a better name than bit-is-set()?
***** Sec. 13
      I don't know how how important it is to have the ABNF
      unambiguous. Previously, Jernej pointed out that refine-stmt
      makes the grammar ambiguous:

      http://www.ietf.org/mail-archive/web/netmod/current/msg07740.html

      The if-feature-expr production is another source of
      ambiguity. An unambiguous set of productions could look like
      this:

      if-feature-expr = if-feature-disj
      if-feature-disj = if-feature-conj
      if-feature-conj = identifier-ref-arg
      if-feature-expr = if-feature-expr 'or' if-feature-disj
      if-feature-disj = if-feature-disj 'and' if-feature-conj
      if-feature-conj = 'not' if-feature-conj / '(' if-feature-expr ')'

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

---

From: Martin Bjorklund <[email-masked]>
Date: Fri, 28 Nov 2014 12:29:16 +0100 (CET)

Lada,

Thank you for this review!  Comments inline.

>      case is created, ...". Sec. 3 should probably also define what

(This text hasn't changed from RFC 6020)

How about: "When a node from one case is created in the data tree,
..."?

>          A leaf-list instance is a sequence of values of a particular type.

(This text hasn't changed from RFC 6020)

How about using similar language as in 4.2.2.4 for lists: "A leaf-list
defines a sequence of values of a particular type."

>      this section explaining how "must" expressions on NP-containers

Yes you are right.  Today there is actually some duplication of text
between section 6.4.1 and 7.5.3.  Instead of adding to the
duplication, I suggest we do:

OLD:

   When a datastore is validated, all "must" constraints are
   conceptually evaluated once for each data node in the data tree, and
   for all leafs with default values in use (see Section 7.6.1).  If a
   data node does not exist in the data tree, and it does not have a
   default value, its "must" statements are not evaluated.

NEW:

   When a datastore is validated, all "must" constraints are conceptually
   evaluated once for each data node in the accessible tree (see
   Section 6.4.1).

>      if the two statements are identical.

Ok, I agree.  Phil made the same comment at the mic in Honolulu, so
unless anyone objects, I'll make this change.

>      the node (with no prefix if there is none).

This would break modules like ietf-system, which use the
<prefix>:<name> value.

The idea is that the prefix is the prefix used in the import
statement.

NEW:

   The string value of a node of type identityref in a "must" or
   "when" XPath expression is the referred identity's qualified name
   with the prefix present.  If the referred identity is defined in an
   imported module, the prefix in the string value is the prefix
   defined in the corresponding "import" statement.  Otherwise, the
   prefix in the string value is the prefix for the current module.

>      Consequently,

See above.

>          consecutively against each member type, ...

Ok, fixed.

>        are specified in the same form as in XPath 1.0 spec.

Ok.

>        Functions" and "String Functions" (XPath spec uses these

But then it wouldn't be consistent with the other subsections...

>           returns a node-set that has the context node as its only

Ok, fixed.

>       node-set, string, number or boolean.

I would like to hear other peoples opinion on this.

>       A similar function that's also needed is derived-from-or-self().

Ok, added.

>       Maybe bit() is a better name than bit-is-set()?

I prefer a more descriptive name than just "bit".

is-bit-set()
bit-set() -- not good, sounds like it might set the bit
bit-set-p()
bit-is-set()

>       ambiguity.

Why is it ambiguous?

> An unambiguous set of productions could look like
>       this:
> 

Hmm, this can't be right; you're defining each LHS twice.

/martin

---

From: Ladislav Lhotka <[email-masked]>
Date: Fri, 28 Nov 2014 16:09:11 +0100

> (This text hasn't changed from RFC 6020)

Yes, I know, I just came across it recently.
> How about: "When a node from one case is created in the data tree,

The term "node" is overloaded. The terminology section defines only
"schema node" and "data node" (as a special type of schema node) but
"node" is also often used for refering to an instance (i.e. XML
element). And then XPath uses "node" in yet another sense.

It is confusing but I don't know whether we can do something about it.

> defines a sequence of values of a particular type."

OK.

>    evaluated once for each data node in the accessible tree (see

Sec. 6.4.1 talks about accessible tree, and I think it is not the same
as "data tree" in sec. 7.5.3:

If a data node does not exist in the data tree, and it does not have a
default value, its "must" statements are not evaluated.

That is, one thing is to decide which "must" statements are to be
evaluated, and another thing is to determine the accessible tree for a
particular XPath expression.

I think sec. 7.5.3 should say something like this:

    For validation purposes, the data tree is extended with the following
    default contents:

    o If a node that exists in the data tree has a non-presence
      container as its child, then this container is added if it is
      missing.

    o If a node that exists in the data tree has a leaf as its child,
      and the data model defines a default value for this leaf, then the
      leaf is added if it is missing.

Maybe a special term could be introduced for the extended data tree,
such as "effective data tree".

> unless anyone objects, I'll make this change.

OK. Maybe the same could be done also for if-feature on keys (last
paragraph in 7.18.2). Although here the two conditions must be the same,
I see no point in having if-feature on a key if it is already on an
ancestor node.

> This would break modules like ietf-system, which use the

Yes, in ietf-routing as well. That's why we need to switch to that XPath
function asap.

> The idea is that the prefix is the prefix used in the import

I think this is ill-defined because identityref value is defined to be a
qualified name, and this term has an exact meaning in XPath
(http://www.w3.org/TR/REC-xml-names/#NT-QName):

The Prefix provides the namespace prefix part of the qualified name, and
MUST be associated with a namespace URI reference in a namespace
declaration.

The import statement has no effect in an instance document.

> But then it wouldn't be consistent with the other subsections...

OK.

> bit-set() -- not good, sounds like it might set the bit

bit-is-set() doesn't sound like a question. How about bit-test()?

>>       The if-feature-expr production is another source of

Because you can have two different derivation trees for the same
expression, such as "foo and bar or baz", see

http://en.wikipedia.org/wiki/Ambiguous_grammar

As Jernej pointed out, this is mainly of concern for somebody who wants to
generate a parser directly from the ABNF.

> Hmm, this can't be right; you're defining each LHS twice.

OK, if it is forbidden in ABNF, then it could be rewritten like so:

      if-feature-expr = if-feature-disj / if-feature-expr 'or' if-feature-disj
      if-feature-disj = if-feature-conj / if-feature-disj 'and' if-feature-conj
      if-feature-conj = identifier-ref-arg / 'not' if-feature-conj / '(' if-feature-expr ')'

Lada