Subject: Re: [netmod] Last Call: draft-ietf-netmod-smi-yang-01 (20110704)

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 14 Nov 2011 05:14:46 +0100

On Sun, Jul 24, 2011 at 01:56:22PM -0400, Martin Bjorklund wrote:
> Hi,
> 
> I have reviewed this document, and I think it is ready for
> publication, with some (mostly minor) edits:
> 
> 
> Section 3
> 
>   When you read this document from the beginning, it is not clear what
>   this section is for.  Later in the document there are backwards
>   references to it, so it gets clear.  I suggest to add a sentence in
>   the beginning of section 3, which says something like: "This section
>   describes an algorithm to generate unique module prefixes, to be
>   used in the import statements."
> 

fixed
 
> Table 2
> 
>   Says "YANG / SMIv2 Module".  Should be only YANG module?
> 
>   If so, also
> 
>     s/well known SMIv2 and YANG modules/well known YANG modules/g
> 

fixed

> Section 3
> 
>   This section talks about "module name", "SMIv2 module name".  I
>   think it should only mention "YANG module name".  Now, since the
>   YANG module name is later defined to be the SMIv2 module name, it
>   doesn't really matter for the end result, but it is clearer if the
>   alg. describes how a prefix (which is a YANG construct) is generated
>   from the YANG module name.
> 
> 
>   s/by tokenizing an SMIv2 module name/
>     by tokenizing the YANG module name/
> 
>   s/The tokens derived from a module name/
>     The tokens derived from the module name/
> 
>   s/shortest sequence of token/
>     shortest sequence of tokens/
> 
>   s/at least two token/at least two tokens/

fixed

> Section 4
> 
>   s/constant prefix, which followed/
>     constant prefix followed/

fixed 
 
> Table 3
> 
>   This table explicitly lists some SNMPv2-SMI symbols, and then
>   there's a catch all clause that all SNMPv2-SMI symbols are ignored
>   during import.  I suggest the explicit symbols are removed.

fixed by adopting David's rewrite of the import algorithm

> Table 3
> 
>   This table and following text says that all symbols in
>   SNMPv2-CONF are translated by translation rules.  But there are no
>   translation rules for the macros in SNMPv2-CONF.  I suggest you add
>   a section that describes how these macros are handled - i.e., that
>   they are not translated at all.

fixed by adopting David's rewrite of the import	algorithm
 
> Table 3
> 
>   This table lists 'snmpTraps' from SNMPv2-MIB.  But why is that one
>   special?  Suppose someone imported 'snmpMIBObjects' from
>   SNMPv2-MIB.  Why is this different?

fixed by adopting David's rewrite of the import	algorithm
 
> Section 4
> 
>   "according to the type mapping table"
> 
>   Add a reference "according to the type mapping table (see table 1)"

fixed 
 
> Section 4
> 
>    "This requires to consider all the types used in the translation
>    unit"
> 
>   What is a translation unit?

I changed the working to avoid this term.
 
> Section 4.1
> 
>    "The translation of the IF-MIB [RFC2863] leads to the YANG module
>    frame"
> 
>   What is a module frame?

OK, I will try to be more explicit to avoid new undefined terms.

> Section 5.1
> 
>    "Furthermore, SMIv2 modules generated from SMIv1 modules may miss an
>    invocation of the MODULE-IDENTITY macro as well"
> 
>    So what will the top-level container be in this case?
> 
>    Since the concept of a top-level container is used later in the
>    translation, this needs to be specified.

I have created an open issue for this. The likely resolution is that
we use the SMIv2 module name as the top-level container.

> Section 7
> 
>   "OBJECT IDENTIFIER assignments are not translated into YANG
>   statements"
> 
>   Isn't this what the new smiv2:alias statement was supposed to be
>   used for?

Will change if we make smiv2 mandatory to generate. Open issue for now.

> Section 8.1
> 
>    "All leaf statements for scalar objects are created in the top-level
>    container"
> 
>   OK -- but I just want to point out that this leads to a model where
>   you need to be careful with using filters if you don't want to get
>   all data for a module.

yep - created an open issue for it.

> Section 8.4
> 
>      container ifTable {
>        config false;
> 
>   where did the config false statement come from?
> 
>   Ok, it's not wrong, but unnecessary.  It's perfectly fine if your
>   implementatiomn generates it, but it should be removed from this
>   example, otherwise a reader might think he missed something.

Copied from smidump - removed. 
 
> Section 8.6
> 
>   I think the name, description, reference and value of the augmenting
>   OBJECT-TYPE should not be captured in comments, but in a smiv2:alias
>   statement.  For example:
> 
>   OLD:
> 
>      /*
>       * ifXTable (1.3.6.1.2.1.31.1.1)
>       *
>       * A list of interface entries.  The number of entries is
>       * given by the value of ifNumber.  This table contains
>       * additional objects for the interface table.
>       */
> 
>   NEW:
> 
>     smiv2:alias ifXTable {
>       smiv2:oid "1.3.6.1.2.1.31.1.1";
>       description
>         "A list of interface entries.  The number of entries is
>         given by the value of ifNumber.  This table contains
>         additional objects for the interface table.";
>     }

Will likely be done, David asked for this as well.
 
> s/""/"/g

fixed

> Section 9.
> 
>    The SMIv2 uses invocations of the OBJECT-IDENTITY macro to define
>    information about an OBJECT IDENTIFIER assignment.  Invocations of
>    the OBJECT-IDENTITY macro MUST be translated into YANG identity
>    statements as detailed below.
> 
>   Ok, but in this case, from the MALLOC-MIB, this will be somewhat
>   odd...
> 
> madcapConfig OBJECT-IDENTITY
>     STATUS     current
>     DESCRIPTION
>             "Group of objects that count various MADCAP events."
>     ::= { madcap 1 }

I do not think something can be done about this.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1, 28759 Bremen, Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>