Subject: [Ips] MIB Doctor review (part-1) for: draft-ietf-ips-isns-mib-09.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Wed, 10 May 2006 23:16:51 +0200

Dan asked for a volunteer for MIB doctor review, and
I offered to do so. Here is my review part-1:

- Syntax checking
  SMICng tells me:
    W: f(isns.mi2), (2626,19) Row "isnsRegFcNodePortEntry" does not 
       have a consistent indexing scheme - cannot specify an index
       item from additional "base row" isnsRegFcNodeEntry, since 
       can have only one "base row" which is isnsRegFcPortEntry
    W: f(isns.mi2), (294,7) Textual convention "IsnsNodeIndexIdOrZero"
       defined but not used

Both are probably OK as long as you are sure that this is what you
intend for the first warning.
For the second warning one could wonder wht the TC was defined if
it is not (yet?) used. Maybe another MIB module is using it?

- smilint has no complaints.

- I am somehwat confused by:
      IsnsEntityProtocolId ::= TEXTUAL-CONVENTION
          STATUS         current
          DESCRIPTION
      "The type of protocol that is supported by this entity.

                 Type Value       Entity Type
                 ----------       -----------
                    1             No Protocol
                    2             iSCSI
                    3             iFCP
                  All Others      As in the iSNS Specification
      "
          REFERENCE      "RFC 4171, Section 6"
          SYNTAX         INTEGER { noProtocol(1),
                                   iSCSI(2),
                                   iFCP(3) }
  Since this is an ENUMERATION, I have difficulty understanding what
  "All Others" means in the DESCRIPTION clause.
  Now if I go to the RFC4171, then I see that IANA assigns new values (and so
  I think that that is meant here). So I then wonder if it would not be better
  to move this to an IANA maintained MIB module that is kept in sync with the
  registry that IANA already must maintain, i.e. the registry at
  http://www.iana.org/assignments/isns-parameters ?

- I also get confused by:
      IsnsPortalGroupTagIdOrZero ::= TEXTUAL-CONVENTION
          DISPLAY-HINT   "d"
          STATUS         current
          DESCRIPTION
      "The Portal Group Tag (PGT) TC for iSCSI Portal Group
       objects registered in the iSNS.  The value of zero
       indicates a NULL value, or no association, between the
       associated Portal and iSCSI Node."
          REFERENCE      "RFC 4171, Section 6"
          SYNTAX         Unsigned32 ( 0 .. 65535 )
   Sect 6.5.4 of 4171 claims that zero is a valid PGT value/ID,
   and that a NULL PFT would be expressed by using a zero length
   in a TLV. So is the use of zero here in conflict with that sect 6.5.4?
   If not, pls explain why not (and do so in the DESCRIPTION clause.

- When I see       IsnsPortalSecurityBitmapId ::= TEXTUAL-CONVENTION
  Then I first wonde if this is an "Id" (Identifier?) of if the name
  would better be
                   IsnsPortalSecurityBitmap   ::= TEXTUAL-CONVENTION
  But I am more worried about the fact that the bit numbers are different
  from what is described in sect 6.3.9 of RFC4171. If the WG wants to
  do it this way conscuiously, then fine, but imagine what happens if
  other bits get used in the fture (say 23 and 24) and we map those to
  bits 7 and 8 in the TC, and then yet later bits 21 and 22 get used
  and we map them to bit 9 and 10. Won;t that start to be confusing?
  Would it not be handier to define it as
          SYNTAX        BITS {
                            reserved0(0),
                            reserved1)1),
                            ...
                            reserved24(24),
                            tunnelModePreferred(25),
                            transportModePreferred(26),
                            pfsEnabled(27),
                            agressiveModeEnabled(28),
                            mainModeEnabled(29),
                            ikeIpsecEnabled(30),
                            bitmapVALID(31)
                             }
  So that it maps directly onto the bits in 4171 sect 6.3.9 ??

- for IsnsNodeIndexIdOrZero I guess that the value zero often means
  none, i.e. no NodeIndexID exists. But I could see it means
  something else depending on the object that uses this syntax.
  I would suggest to change the DESCRIPTION clause  to something aka:

        "This textual convention is an extension of the IsnsNodeIndexId
         textual convention.  The latter defines a greater than zero 
         value used to identify an IsnsNode.  This extension permits the
         additional value of zero.  The value zero is object-specific
         and MUST therefore be defined as part of the description of
         any object which uses this syntax.  Examples of the usage of
         zero might include situations where the IsnsNode was unknown,
         or when none or all IsnsNodes need to be referenced."

- IsnsNodeTypeId is it an Id (Identifier?)? or is it actually a map
  (or list) of nodeTypes? Good names make sense in my view.
  Again, I wonder if mapping it to actualy the same bit positions as in
  RFC4171 would not be better.

- IsnsCosBitmapId is it an Id (Indetifier?)?
  Same question on mapping bits

- Same for IsnsScnBitmapId

- Same for: IsnsSrvrDscvryMthdId
  Seems like a map of (supported?) methods as opposed to an ID.

- When I see:
      isnsSrvrInstPhyIndex        OBJECT-TYPE
          SYNTAX                  Unsigned32 (0..2147483647)
          MAX-ACCESS              read-only
          STATUS                  current
          DESCRIPTION
      "An index indicating the location of this iSNS Server within
       a larger entity, if one exists.  If the iSNS Server instance
       is not part of a larger entity, then the value is 0."
          REFERENCE               "RFC 4171"
          ::= { isnsSrvrInstEntry 5 }
 
  Then I am not sure how this "index" tells me anything about the
  location within a larger entity. Possibly it dioes because it is
  an index into some other table, but can you pls specify which table
  that would be. If it is not an index into some other table,
  then pls explain how it helps in determining "location"?

- Why does
      isnsSrvrInstRole            OBJECT-TYPE
           SYNTAX                 INTEGER { notSet(0),
                                            server(1),
                                            serverNotPrimary(2) }
  not start ENUMerating at 1 instead of zero.
  We recommend starting at 1 unless there is a good reason to start
  at zero (which we then like to see mentioned in the DESCRIPTION clause)
  I can't find why starting at zero is required. 
  Is there any specific section in RFC4171 about this?
  I see a section on bacup (2.8), that speaks about an "active server"
  which I do not see mentioned here. Is "serving as a primary"
  teh same as "active server" ?? That section also speaks about
  "backup server" which I do not see here? The section indeed also
  speaks about a "primary server"
  In any event, I am not sure if and how this object is related to
  section 2.8. Maybe it is not and maybe it is related to something else?

- isnsSrvrInstDiscMcGrp
  Whever you define an object with SYNTAX of InetAddress, then (according
  to the DESCRIPTION clasue of InetAddress in RFC4001), you MUST state 
  WHICH object of SYNTAX InetAddressType specifies the format of this
  object. It seems obvious that this is isnsSrvrInstDiscMcGrpType, yet
  it is good to mention that. 
  Further, it states:
       for this server instance.  If not configured, then
       the value is an empty string."
  But, if it is not configured, then the isnsSrvrInstDiscMcGrpType has
  a value of unknown (or so I assume), and the value of this object then
  better be the "zero length string" as opposed to "empty". What does
  "empty" mean?

  I would personally rename isnsSrvrInstDiscMcGrp to isnsSrvrInstDiscMcGrpAddress

- W.r.t. isnsSrvrInstDiscMcGrpType and isnsSrvrInstDiscMcGrp, I think 
  one could say some more about the allowed InetAddressTypes (if not in the
  DESCRIPTION clauses of these objects themselves, then at least in a 
  OBJECT clause in the MODULE-COMPLIANCE statements. 
  If I understand things correctly, it has to be an IP multicast address,
  so possibly only the types "unknown", "ipv4" and "ipv6" are allowed?
  If "dns| is allowed, then you need to add text as to when a DNS name
  would be resolved (as per RFC4001).

- isnsSrvrInstEsiNonRespThrshld, isnsSrvrInstEnblCntrlNdeMgtScn and 
  isnsSrvrInstCntrlNodeAuth, isnsSrvrInstDfltDdDdsStatus and
  isnsSrvrInstUpdateDdDdsSpprtd and a few more that follow
  These objects have a          REFERENCE "RFC 4171, Section 3.4"
  but maybe you mean sect 2.4 ??

- I can't say that I find the DESCRIPTION clause for isnsSrvrInstCntrlNodeAuth
  very well written. I still need to review the other tables it is pointing
  to, so I can't say much more yet.

nits/typos/consistency/questions:

- I wonder why       IsnsDdsStatusId ::= TEXTUAL-CONVENTION
  is not just named  IsnsDdsStatus   ::= TEXTUAL-CONVENTION
  I.e. I do nto see why it is an Id (Identifier?)??
  Further,       IsnsDdsStatusId ::= TEXTUAL-CONVENTION
          STATUS         current
          DESCRIPTION
      "The bitmap indicating the status of a Discovery Domain
       Set (DDS) registered in the iSNS.
                    Bit           Status
                 ---------       ---------
                     0            enabled

       If bit(0) is set to true then the DDS is Enabled.  Otherwise
       the DDS is disabled."
          REFERENCE      "RFC 4171, Section 6"
          SYNTAX         BITS {
                            enabled(0)
                              }
   "If bit(0) is set to true" ??? I understand what is meant.
   But I think it would be cleared to just say
   "If bit(0) is set to 1"  or "If bit(0) is set"
   Or/and be consistent with how you describe the setting of a bit
   with other BITS TCs like DdFeatureBitmapId

- For consistency, I would rename    DdFeatureBitmapId ::= TEXTUAL-CONVENTION
  to                                 IsnsDdFeatureBitmapId ::= TEXTUAL-CONVENTION
  or even better:                    IsnsDdFeatureBitmap   ::= TEXTUAL-CONVENTION
  again, I do not see how this is an Id (Identifier?)??

- isnsSrvrInstEsiNonRespThrshld ... is this an Id (Indetifier?) or is it a threshold.
  From the descritpion clause it seems it is the latter. So I would rename to
  isnsSrvrInstEsiNonRespThrsh 
  Mmm... now I see, the l is probably an el and not a one.
  Why abbreviate "hold" to "hld" ??
  In fact why abbreviate "Threshold" to "Thrshld". We (readers) are not all Americans
  or native English speakers.  In fact this whole doc uses (for my taste) far to
  much (strange) abbreviations for object descriptors and labels. But who is me.

- isnsSrvrInstUpdateDdDdsSpprtd and isnsSrvrInstUpdateDdDdsSpprtd
  use a TC for theri SYNTAX. The idea of a TC is that you only define the
  semantics in teh DESCRIPTION clause of the TC so you do not have to
  repeat it everytime that the TC is used as a SYNTAX.

admin/bureaucracy:

- You may want to check the occurences of "MIB", which in many cases woul be
  better stated as "MIB module".

- references/citations:
  !! Contains embedded space:
  P001 L134:     network [RFC 4171].  It has the capability to group devices into

  !! Contains embedded space:
  P001 L264:     Specification [RFC 4171], a DDS can be enabled or disabled,

  !! Contains embedded space:
  P001 L307:     As described in iSCSI [RFC 3347], Portal Groups provide a

  The first two are indeed just what it says, namely a blank in between
  RFC and the actual number. In the references section, you list it as [RFC4171]
  without a space.

  The 3rd does have an embadded space too, but also, that RFC does not
  show up in the references section.

_______________________________________________
Ips mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/ips