Subject: [netmod] Review of draft-linowski-netmod-yang-abstract-03.txt

From: Martin Bjorklund <[email-masked]>
Date: Mon, 30 Aug 2010 12:45:55 +0200 (CEST)

Hi,

I have reviewed the document
draft-linowski-netmod-yang-abstract-03.txt.  I have ( tried to :) stay
away from my opinion on the features introduced; I think I have made
those clear already.

------------------------------------------------------------

s 1.3.  bullet 3.

 s/It is not necessary to explicitly add a key
      statement to containers/
   It is not necessary to explicitly add a key
      statement to lists/

------------------------------------------------------------

s 1.5.1.  yang module udmcore

Is this module supposed to be an example?  If so, the namespace should
probably be something like http://example.com/udmcore.

Same comment applies to modules "hardware-entities" and "hw".

------------------------------------------------------------

s 2.2.

In the list of statements and cardinality in this section and others,
I suggest you use the prefixed notation for the new statements, to make
it easier to follow.  E.g.:

OLD:
                      +---------------+-------------+
                      |  substatement | cardinality |
                      +---------------+-------------+
                      |    abstract   |     0..1    |
                      |     anyxml    |     0..n    |
 
NEW:

                      +---------------+-------------+
                      |  substatement | cardinality |
                      +---------------+-------------+
                      |  ct:abstract  |     0..1    |
                      |     anyxml    |     0..n    |

  etc

------------------------------------------------------------

s 2.2.

   Complex-type definitions do not create nodes in the management
   information tree.

The term "management information tree" is not defined.  It is used in
other places as well.  Did you mean "schema tree", as defined in YANG?

------------------------------------------------------------

s 2.3, 2.4.

The lists of substatements include "type", I think it should be
ct:instance-type.

------------------------------------------------------------

s 2.4.

   Common YANG statements may
   be used as substatements of the "element" statement.  

What is the "element" statement?

The term "element" is used in other places as well, e.g.:

   In case the instance list represents configuration data, the used
   complex type of an element MUST have an element key.

I suggest you go through the document and change this to the
appropriate term.

------------------------------------------------------------

s 2.6.

OLD:

   The complex type serves only as a base type for derived concrete
   complex types and cannot be used as a type for an instance in NETCONF
   payload.

NEW:

   An abstract complex type serves only as a base type for derived
   concrete complex types and cannot be used as a type for an instance
   in NETCONF payload.

------------------------------------------------------------

s 2.8.

OLD:

   The suggested namespace prefix is "ct".

I suggest you use the prefix "cti", in order to not confuse
complex-type-instance with complex-type.

------------------------------------------------------------

s 2.8.

It seems the type encoding rules impose a strict order among the
XML elements.  Note that YANG does not impose such a strict order, so
I suggest you clearly state that these rules change the rules defined
by YANG.

Also, what happens if class B derives from class A, and class B
defines the keys?  In YANG, the keys are always encoded first, but
this text says that class A's leafs are encoded before class B's
leafs.  This must be clarified.

Will the meta-data "cti:type" element be encoded before the keys?
This must be clarified.

------------------------------------------------------------

s 2.8.

You are defining new NETCONF error-tags.  I think you meant to use
error-app-tag.  I think the error-tag should be "bad-attribute", and
error-app-tag "wrong-type" (or maybe "wrong-complex-type").  For
"missing-type", I suggest you instead use the standard error-tag
"missing-attribute".

------------------------------------------------------------

s 2.9.

Why do you define the "complex-type" feature?  Why is this feature
important to a client?  How will the client adapt depending on if this
feature is present or not?

IMO, it is not needed at all.  If the server advertises support for a
module that uses complex-types, it obvioulsy implements complex-types.

------------------------------------------------------------

s 2.9.

Change the name of "complex-types" to "ietf-complex-types" or
"ietf-experimental-complex-types".

------------------------------------------------------------

s 2.12.

OLD:

   o  New data definition statements MAY be added to a complex type only
      if:

      *  they are not mandatory and

      *  they are not conditionally dependent on a new feature (i.e.,
         have an "if-feature" statement, which refers to a new feature).

This is probably not correct.  I think you mean:

NEW:

   o  New data definition statements MAY be added to a complex type only
      if:

      *  they are not mandatory or

      *  they are conditionally dependent on a new feature (i.e.,
         have an "if-feature" statement, which refers to a new feature).

------------------------------------------------------------

s 2.13

  s/namespace/XML namespace/ ?

------------------------------------------------------------

s 2.13.2

Is it correct that if one uses recursive complex types, then the
resulting hiearchy cannot be augmented?  Isn't this a pretty severe
restriction?  If nothing else, I think this restriction should be
noted when recursive complex types are defined.

------------------------------------------------------------

s 2.13.3

   A module might not want to support all complex types derived from a
   base class defined in another module.

Did you mean s/module/server/?  Note that deviations are used to
indicate how a server implementation deviates from a module.
Deviations cannot be used in normal modules.

------------------------------------------------------------

s 3.1

   Typed instance identifier relationships are an addition to the
   relationship types already defined in YANG, where the leafref
   relationship is location dependent, and the instance-identifier is
   not type-safe.

Please explain what you mean with "type-safe".

It is true that there is no formal way of specificying what an i-i can
point to in YANG, but it can be defined in the description statement.

------------------------------------------------------------

s 3.2.

Please explain the term "scoped name".

------------------------------------------------------------

s 3.2.
 
   This extension statement MUST be used as a
   substatement of the instance-identifier statement.

There is no "instance-identifier" statement.  Please clarify.

------------------------------------------------------------

Appendix B and C

The modules in these Appendicies import "yang-types" and
"inet-types".  These should be "ietf-yang-types" and
"ietf-inet-types".  Also, the prefix for ietf-yang-types should be
"yang" (not "yt").

------------------------------------------------------------

Appendix C.1

Second bullet says:

   o  All associations between the concepts (which are not containment)
      are represented with typed identifiers.  That avoids having to
      refer to a particular location in the tree, which is not mandated
      by the original model.

To be fair; the original model doesn't say that the reference is
supposed to be to any place in the tree either.

In general, I have a hard time appreciating this (location independent
reference) as a feature.  I think it introduces more problems than it
solves, and there are other ways to solve the underlying problem.

------------------------------------------------------------

If complex-types are used, some of the flexibility of YANG is lost.
For example, in your equipment example, suppose vendor X wants to add
its own proprietary attributes to all "ManagedHardware".  This will
not be possible unless all classes derived from ManagedHardware at
some point are tracked down, and all ct:instance statements for these
classes are augmented with the new attributes.

------------------------------------------------------------

I have checked the main module, "complex-types", and the example
modules using pyang.

The module ipfix-psamp has a couple of errors:

  -  The type autonomous-system-number is not found (it is called
     as-number).

  -  It uses the statement "type" instead of ct:instance-type.

  -  There is a syntax error:
      type instance-identifier {ct :instance-type PhysicalEntity; }
                                ^^^^^^

/martin