Subject: review of draft-ietf-mpls-ldp-mib-09.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 6 Dec 2002 23:44:26 +0100

Here we go (mix of nits and some serious)

- missing IPR section
- Security considerations probably needs more work
  Security ADs want you to explain what the vulnerabilities/risks
  are and what to do against them.
  Also for read only objects, pls list each (group of) object(s)
  and explain what sensitivity attributes they have
- pls do the consistency checking for descriptors and all that
- sect 3.5 1st sentence
  s/would be/are/ ??
- sect 3.5   2nd para first sentence
  s/initiation/initiate/ 
- section 3.5.2 
  s/mpsl/mpls/
- mplsXxxIndexNext
  See my comments on FTN MIB and LSR MIB on these type of objects
  Best to use something aka rfc3289
- RowStatus and StorageType objects
  see my comments about similar objects in LSR MIB
  for StorageType might also want to add a DEFVAL 
- mplsLdpNotifications ... { mplsLdpMib 2 }
  why not { mplsLdpMib 0} so that it is right away the prefix?
  I believe LSR MIB does it that way now. You may want to do
  it consistently for all notifications.
  I can live with either way, but prefer them to be shorter OIDs.
- mplsXxxLastChange
  does that time stamp only get changed if an addition/deletion takes
  place? Not if something gets changed via a SNMP SET command?
  I think I'd prefer to also see changes (modifications) via SET.
  But in any event, be very explicit about if those are included or
  not.
- mplsLdpLspType
  All that stuff in the DESCRIPTION clause is just a repeat of the
  TC DESCRIPTION clause. Seems not needed to me. What if a value
  gets added later... how do you stay in sync?
- mplsFecAddrLength
  Should that be of SYNTAX InetAddressPrefixLength as per RFC3291?
- mplsFecAddrFamily and mplsFecAddr
  These are strange. The DESCRIPTIONS are certainly not meeting the
  requirements as specified in RFC3291. At other places you do it
  correct, so you do understand what is required I think
- mplsLdpLspFecTable claims to be a read-only table.
  Yet you have a read-create RowStatus object in it.
- mplsLdpSessionUp and mplsLdpSessionDown
  Is it not better to just have one notification, namely a
  mplsLdpSessionStateChange and then the mplsLdpSesState object will
  explain what the change is?
- COMPLIANCE section. I hope that INT ADs are OK with making IPv6
  addresses optional. Is that cause current LDP only supports IPv4?
  If so you may want to add that as an explanation.

MPLS-LDP-GENERIC-MIB
- same on mplsXXXIndexNext and RowStatus and STorageType objects
- this looks wierd:
    ::= { mplsMib 6 } -- to be assigned
  use cc instead of 6 if you want IANA to assign, and do tell 
    -- to be assigned by IANA
  and write something about it in an IANA Considerations Section if
  this is what you want.
- mplsGenModuleROCompliance
  I would call it mplsGenModuleReadOnlyCompliance
  You have used ReadOnly and Full in other places/mib modules and
  it is good to be clear and consistent

MPLS-LDP-ATM-MIB
- s/Moduel/Module/
- same on mplsXXXIndexNext and RowStatus and STorageType objects
- this looks wierd:
    ::= { mplsMib 4 } -- to be assigned
  use cc instead of 4 if you want IANA to assign, and do tell 
    -- to be assigned by IANA
  and write something about it in an IANA Considerations Section if
  this is what you want.
- See earlier remark on Notifications (use zero instead of 2 right away)
- There are still some INTEGER enumerations that start with zero
  It is not a BLOCKING problem... but if acceptable, pls make it start
  at 1 (as you have done vor various others of these enumerations)
- mplsXxxxROCompliance  -> better mplsXxxxReadOnlyCompliance
- some formatting problems with DEFVAL lines being split on 2 lines?

MPLS-LDP_FRAME-RELAY-MIB
- consistency in descriptors (FrameRelay vs FR etc)
- same on mplsXXXIndexNext and RowStatus and STorageType objects
- this looks wierd:
    ::= { mplsMib 5 } -- to be assigned
  use cc instead of 5 if you want IANA to assign, and do tell 
    -- to be assigned by IANA
  and write something about it in an IANA Considerations Section if
  this is what you want.
- See earlier remark on Notifications (use zero instead of 2 right away)
- There are still some INTEGER enumerations that start with zero
  It is not a BLOCKING problem... but if acceptable, pls make it start
  at 1 (as you have done vor various others of these enumerations)
-    mplsLdpEntityFrLRComponents OBJECT-TYPE
       SYNTAX      Unsigned32 (1..65535)
       MAX-ACCESS  read-create
       STATUS      current
       DESCRIPTION
           "Number of LabelRange Components in the Initialization
           message.  This also represents the number of entries
           in the mplsLdpEntityConfFrLRTable which correspond
           to this entry."
   Where is this mplsLdpEntityConfFrLRTable ??
- I see the xxxDlci index objects start at zero.
  Pls add to DESCRIPTION clauses why zero must be an index.
- mplsXxxxROCompliance  -> better mplsXxxxReadOnlyCompliance

Thanks,
Bert