The MPLS WG Archive

["Wijnen, Bert (Bert)" <bwijnen@lucent.com>]

First a few serious things that need fixing (or a acceptable
answer):

1. All the documents that contain a MIB module from which
   you IMPORT anything must be listed as normative references.
   I think a few are now under informative and a few may even
   be missing.

2. I see at several place in DESCRIPTION clause of StorageType
   objects this text:
      DESCRIPTION
       "The storage type for this conceptual row.  Conceptual rows
        having the value 'permanent(4)' MAY allow write-access to any
        columnar objects in the row, except for setting the
        mplsLdpEntityRowStatus to 'destroy(6)'."
   And that is very weird. It seems to allow to change the storagetype
   object itself (which is a nono). And the cannot destroy is also
   already covered by being a permanent entry.
   The required text from RFC2579 (Storage Type TC) is something aka:
       "The storage type for this conceptual row.  Conceptual rows
        having the value 'permanent(4)' need not allow write-access
        to any columnar objects in the row."
   Which you have in some other cases, and which I think expresses
   the same and is correct accoring to RFC2579 requirements.
   mplsLdpEntityAtmLRStorageType has it specified correctly!

3 I see:
         mplsFecAddrPrefixLength    InetAddressPrefixLength,
         mplsFecAddrFamily          InetAddressType,
         mplsFecAddr                InetAddress,
  I believe that RFC3291 suggests to put InetAddressType BEFORE
  any objects that are to be intepreted within the context of
  the specific addressType. so the sequence should probaly be:
         mplsFecAddrFamily          InetAddressType,
         mplsFecAddrPrefixLength    InetAddressPrefixLength,
         mplsFecAddr                InetAddress,

  I would also check the DESCRIPTION clauses with RFC3291.
  I am not so sure it is all in sync with prescriptions of RFC3291
  I am specifically worried about the reference to RFC1700 (which is
  obsolete anyway, probably you better refer to
      http://www.iana.org/assignments/address-family-numbers
  if that is what you want). But is it not just IPv4 and IPv6
  addresses that you support (at least that is what compliance
  seems to state?
  In the DESCRIPTION of PrefixLength I would change 
            "If the value of the 'mplsFecType' is 'hostAddress(2)'
            then this object is undefined.
  Into either:
            "If the value of the 'mplsFecType' is 'hostAddress(2)'
            then the value of object is irrelevant and ignored.
  Or (maybe better, not sure):
            "If the value of the 'mplsFecType' is 'hostAddress(2)'
            then the value should be equal to the length of the
            address in mplsFecAddr.
  Also in that same DESCRIPTION clause:
             prefix matches all addresses.  In this case the
             prefix MUST  also be zero (i.e. 'mplsFecAddr' will
             have the value of zero.)"
  What is a "value of zero"? I think you mean zero length.
  But in order for that to be valid for mplsFecAddr, the
  mplsFecAddrFamily value should be zero (i.e. unknown).

4. How many notifications can be generated per second?
   I worry based on the description clauses. I see a risk that it
   could be many if not implemented properly or with a proper
   throttle? Can you add some text about that?

5. I worry about a statement in a RowStatus DESCRIPTION clause
   (it is doen several times):
            NOTE:  This RowStatus object should
            have the same value of the 'mplsLdpEntityRowStatus'
            related to this entry."
   I will agree that that is the ideal situation. But the rowStatus
   should always represent the real status of the row itself.
   If that is not consistent with some other row in another table,
   then that is bad, but it should not be a cause for the 
   rowStatus of thos column to take an invalid value. I guess
   you did not mean to say it should... but that is how people
   might read it. 


6. MPLS-LDP-GENERIC-MIB
   W: f(mplsldpgen.mi2), (58,17) The first revision should
      match the last update for MODULE-IDENTITY mplsLdpGenericMIB
   I think you just mistyped 2002 instead of 2003.

Nits (some more serious than otehrs)...
would be nice if they can be fixed:

- I would not use capitals for FULL, READ-ONLY, NEW and such.
  There is the risk that someone wants to know if they have
  special meaning.
- sect 3.5 under NOTE: This table
  My question: which table?
- You have in many places a reference aka:
     REFERENCE
       "[RFC3036] LDP Specification, Section on LDP Identifiers."
  Now, once people extract the MIB module from the document (and many
  people do) then you loose the citation. So I always recommend:
     REFERENCE
       "RFC3036, LDP Specification, Section on LDP Identifiers."
- I see:
     mplsLdpPeerTransportAddrType OBJECT-TYPE
         SYNTAX      InetAddressType
         MAX-ACCESS  read-only
         STATUS      current
         DESCRIPTION
             "The object specifies how to interpret the address
             for the mplsLdpPeerTransportAddr object."
     .. snip 

     mplsLdpPeerTransportAddr OBJECT-TYPE
         SYNTAX      InetAddress
         MAX-ACCESS  read-only
         STATUS      current
         DESCRIPTION
             "The transport address advertized by the peer
             in the hello message or the Hello source address."

   I think that the DESCRIPTION for InetAddress object MUST contain
   the explanation how it is interpreted. The example in RFC3291 is
   so explicit and clear (or so I think). I also think that it is
   in fact an Internet address and not a transport address (the
   latter needs a port number too, does it not? see RFC3419)

   So something aka the next seems better (maybe even rename
   TransportAddr to IntenetAddr:

     mplsLdpPeerTransportAddrType OBJECT-TYPE
         SYNTAX      InetAddressType
         MAX-ACCESS  read-only
         STATUS      current
         DESCRIPTION
             "The type of Internet address for the
              mplsLdpPeerTransportAddr object."
     .. snip 

     mplsLdpPeerTransportAddr OBJECT-TYPE
         SYNTAX      InetAddress
         MAX-ACCESS  read-only
         STATUS      current
         DESCRIPTION
           "The Internet address advertized by the peer in the hello
            message or the Hello source address. 

            The type of this address is determined by the value of
            the mplsLdpPeerTransportAddressType object."

  If you do change this, pls check all your InetAddress objects

- mplsLdpSesKeepaliveTime
  What does value of zero mean?

- mplsLdpHelloAdjacencyHoldTimeRem
  could use a UNITS clause
  I also wonder, since 0xffff has a special meaning,does that mean
  that that is also the max value? If so, a range of (0..65535) 
  would make good sense.

- I see:
       mplsOutSegmentLdpLspTable OBJECT-TYPE
         SYNTAX      SEQUENCE OF MplsOutSegmentLdpLspEntry
         MAX-ACCESS  not-accessible
         STATUS      current
         DESCRIPTION
             "A table of LDP LSP's which
             map to the InSegment Table in the
             the LSR MIB's."
  s/InSegment/OutSegment/ ??
  s/MIB's/MIB/ ??

- I may have said this before. Could the mplsLdpSesUp and mplsLdpSesDown
  notification not be combined to mplsLdpSesStateChange?
  The included mplsLdpSesState object will identify if it is an Up or a
  Down, will it not?

- In the DESCRIPTION clauses of your OBJECT-GROUP definitions, you have
  langauge that talsk about "optional" and/or "required to implement".
  That is not the goal of the OBJECT-GROUP macro. It is needed to GROUP
  objects that logically belong together. So the DESCRIPTION should
  have text to explain why these objects belong in one group.
  The "optional" and "required" and susch is language that goes
  in DESCRIPTION clauses of the MODULE-COMPLIANCE GROUP clauses.

- Sect 4.1 talks about mplsLdpEntityAtmLabelRangeTable while the actual
  table name is: mplsLdpEntityAtmLRTable
  Same for FrameRelay and Generic MIB

- mplsLdpEntityAtmTable
  You talk about "extends". I think what the table does is that it
  is a "sparse augments". Your indexes are OK though.

- mplsLdpEntityAtmLREntry talks about overlaps and such that may
  (should, can) not occur. But it does not say anything about what
  sort of error to return if someone tries to SNMP SET (or create)
  incorrect values. And It seems like 2 errors are possible:
  - inconcsistentName (if it is part of the (not-accessible) index)
  - inconsistentValue (if it is a value in read-create column)
  Not 100% sure... anyway, better to specify what you want to 
  happen than let people guess

- mplsLdpEntityAtmLRRowStatus has in DESCRIPTION clause
            There must exist at least one entry in this
            table for every LDP Entity that has
            'mplsLdpEntityOptionalParameters' object with
            a value of 'atmSessionParameters'.
  Does not seem right in RowStatus object. Maybe in the Entry
  or Table object?
  I have seen this in multiple RowStatus Objects

- mplsLdpAtmSesTable talks about  'mplsLdpSessionTable' 
  In fact many places in the doc talk about it.
  Seems you renamed the table to mplsLdpSesTable (not sure why,
  cause I find the full Session better... but that is another
  subject).

- mplsLdpEntityAtmLRRowStatus talks about
      'mplsLdpEntityOptionalParameters'
  and so do some other places, but I cannot find the latter object
  
- I can live with the security considerations section.
  You might be able to shorten it somewhat if you take the last 3
  paragraphs of each 9.x section and state that once in its own
  9.x section. Up to you.

Thanks,
Bert