Subject: Review of: draft-ietf-mpls-telink-mib-00.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 25 Apr 2003 15:54:58 +0200

In reality, I did review a pre-release of rev 01,
but that rev was pretty much the same as rev 00.

Here is my review:

- compiles/syntax-checking clean. Good!

But.....

- linelegth checker:
   $ /bin/checkpage.awk <home/ietf/drafts/draft-ietf-mpls-telink-mib-01.txt
   Long line at 770 with 77 chars
   Long line at 771 with 76 chars
   Long line at 1268 with 73 chars
   -: 3 lines longer than 72 characters, max 77

- Last sentence of abstract talks about "draft". I think you want to use
  "document" or "memo" so that it is still valid when that draft becomes
  an RFC. Maybe this occurs at other places too, pls check
	
- You seem to be using "MIB" many times where "MIB Module" or "MIB document"
  would be better. Remember there is only one (Single) MIB, which is composed
  of many (multiple) MIB modules. One or more MIB modules may be described
  and specified in a MIB document

- I have not yet checked sect 7.

- I do not see any words on the relationship betwen teLinkXxxTables
  and componetXxxxTables. Would be good to write something about that

- I would suggest that maybe you should replace your 1st figure in sect 8.2
  with the figire from the overview document (revision 4) section 11.2. That
  seems clearer to me.

- I still have the impression that sect 5.1 (summary) and section 6 are
  basically a duplicate of each other. I can live with it... but ???

- sect 8.1 under ifType you refernece [IanaFamily].
  I think the reference should be to the ianaiftype.mib 

- for my understanding, the figure in sect 8.2, can you explain which layers
  in the stack are bundle(s), telink or component link. In fact it might be
  good to add that explanantion to the text, so that people can read it too.

- In the CONTACT-INFO, pls add WG mailing list information

- In DESCRIPTION clause of MODULE-IDENTITY..
  - add year (2003) to copyright statement
  And I think I would change:

        This MIB contains managed object definitions for
        MPLS traffic engineering links as defined in:
        Kompella, K., Rekhter, Y., Berger, L.,
        Link Bundling in MPLS Traffic Engineering
        Internet Draft <draft-ietf-mpls-bundling-04.txt>,
        July 2002."

  Into
        This MIB module contains managed object definitions for
        MPLS traffic engineering links as defined in 'Link Bundling
        in MPLS Traffic Engineering'."

- In DESCRIPTION clause of teLinkENtry you still have a (TBD), while
  we already know that it is 200, do we not?

  Also, I do not understand the last sentence of that DESCRIPTION clause
  (I dare to bet there are others who won't understand it either). SO
  please clarify in the text

- May I recommend that 
     teLinkIpAddrType             InetAddressType,
     teLinkIpAddr                 InetAddress,
     teLinkRemoteIpAddr           InetAddress,
  Be renamed to:
     teLinkAddressType             InetAddressType,
     teLinkLocalAddress            InetAddress,
     teLinkRemoteAddress           InetAddress,
  First of all, there can be an unnumbered addresstype (unknown), so then
  it is not an IpAddress. 
  Second, I think that the first address is the local address (that is on
  the device where the instance of the object lives, no? 

  Further I wonder.. if it is an unnumbered link, it has an identifier somehow
  does it not. Why would we not put that identifier in the address in that case
  and describe what it looks like. In fact I wonder if instead of  the
  InetAddress.... TCs would it not be better to use TeHopAddres... TCs from the
  MPLS-TC-MIB? It allows for TeHopAddressUnnum as an address

  In any event, I would not write:
    teLinkIpAddrType OBJECT-TYPE
      SYNTAX        InetAddressType
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
        "For IPv4 and IPv6 numbered links, this object represents the
         IP address type associated with the TE link. For
         unnumbered links, a value of unknown(0) must be used."
  But rather something aka:
    teLinkAddressType OBJECT-TYPE
      SYNTAX        InetAddressType
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
        "The type of Internet address for the TE link. Only IPv4 and
         IPv6 and unknown (for unnumbered links) need to be supported."
  
  Then forther in the Address itself I would do something aka (not that
  the first sentence is REQUIRED as per RFC3291):
    teLinkLocalAddress OBJECT-TYPE
      SYNTAX        InetAddress
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
        "The local Internet address for the TE link. The type of this 
         address is determined by the value of the teLinkAddressType
         object. For an unnumbered TE link, the address represents an
         unnumbered interface in this format:

              octets   contents               encoding
              1-4      unnumbered interface   network-byte order
        "
  I stole some of the text from MPLS-TC-MIB for now

- I see in a lot of places something like:
       REFERENCE
       "[BUNDLING]"
  That is not the proper way to reference in a REFERENCE clause. When a 
  MIB module gets extracted from the RFC (and that happens a lot) then all
  the references to the reference section are lost. So we normally use
  something like:
      REFERENCE "Link Bundling in MPLS Traffic Engineering, RFC aaaa."
      -- RFC Editor to fill in RFC number that will be assigned to [BUNDLING]

  Not that this makes for a normative reference to [BUNDLING]

- I see this coming back a few times:
    teLinkMuxCapability OBJECT-TYPE
        SYNTAX   INTEGER {
                     packetSwitch1(1),
                     packetSwitch2(2),
                     packetSwitch3(3),
                     packetSwitch4(4),
                     layer2Switch(51),
                     tdm(100),
                     lambdaSwitch(150),
                     fiberSwitch(200)
                 }
   So that is a good candidate for a TC. That would then also allow you to
   say a bit more what packetSwitch1, packetSwitch2, etc in fact means.
   Any explanation for skipping values? It is recommended (but not required)
   that they are contiguous. So explaining in the description why such is not
   the case is always wise. It possibly is as simple that these values
   are already allocated/specified somehwere else (GMPLS-OSPF doc?) and 
   reused here.

- Would it make sense to elaborate a bit in the DESCRIPTION clause of
  teLinkProtectionType and explain what the values exactly mean?
  Or if they are explained in that GMPLS-OSPF doc that you refernce, then
  at least make that statement.

- I think all the "priotity" objects have a value from 0-7 (or one that maps
  to it). Does that call for a TC, in which you can then also describe a bit
  more extensive what it is?

- teLinkIncomingIfId and teLinkOutgoingIfId
  I am unclear if these are only for unnumbered links or also for numbered
  ones. Can you make that explicit in the DESCRIPTION clauses?

- The places where you use Priority (1..8) that maps to (0..7), I expect that
  you did that because they are INDEX objects and INDEX objects should not 
  include a zero value. However... that is the RECOMMENDED practice. If there
  is a good reason to include value zero in the index range then such can
  be done, and here it seems that such is justified. (I am assuming here
  that the priority is defined in some other doc as being in the range of
  0-7. Maybe it is even a field in some other protocol data structure.

- teLinkResourceClass
  Any reference to a document that explains how that bit-valued field is
  composed/conbtructed/used?

- In all the places/tables where ifIndex is used as an index, could you
  specify if a specific ifType is expected (or maybe even required) for
  such an interface. I have the impression that for some it is a fixed
  value that is acceprtable, for other multiple values are acceptable,
  and for some maybe even any value is acceptable. If you specified it,
  I think that things would be clearer for me (and hopefully for others
  too).

- At all those places where you use Bandwith of 1000 bits per second.
  You have it in the DESCRIPTION clause. Better is to put it in a UNITS
  clause, so that it is machine readable/parsable.
  It is used at many places. Candidate for a TC ??

- I see this multiple times:
     teLinkEncodingType OBJECT-TYPE
       SYNTAX    INTEGER {
                     packet(1),
                     ethernet(2),
                     ansiEtsiPdh(3),
                     sdhItuSonetAnsi(5),
                     digitalWrapper(7),
                     lambda(8),
                     fiber(9),
                     fiberChannel(11)
                 }
   Yep... as you guessed: why is it not a TC?

- I see a few of these:
   componentLinkPreferredProtection OBJECT-TYPE
      SYNTAX        INTEGER {
                       primary(1),
                       secondary(2)
                   }
  Candidate for a TC I'd say

- I still see in (I believe all of) your RowStatus objects somthing like:
    DESCRIPTION
       "This variable is used to create, modify, and/or
        delete a row in this table. All read-create objects
        can only be changed when teLinkRowStatus is active."
        -----------
  As I said before, This really weird. Probably/Possibly You mean:
        All read-create objects can be changed even when xxxRowStatus
        is active."
  If so, pls fix, if not, pls explain.
  You may want to reread first para on page 17 of RFC2579
  In any event, when a row is initially created, it cannot be 'active',
  it can only become active if all columns have proper values, so one
  must be able to change a row when in notReady or notInservice state.
  Or when in not-existing state. That is the default. The default is also
  that one cannot write (SET) any columns while a row is active, so that 
  is why a DESCRIPTION clause of a RowStatus object MUST specify which
  columns can or cannot be written/changed when the row is active.
  Seems you want to allow all columns to be written-to/changed when a row
  is active, so my suggested text replacement above caters for that.

- WHen I see a table like this:
   teLinkDescriptorTable
   TeLinkDescriptorEntry ::= SEQUENCE {
      teLinkDescriptorId           Unsigned32,
      teLinkEncodingType           INTEGER,
      teLinkDescrPriority          Unsigned32,
      teLinkMinReservableBandwidth Unsigned32,
      teLinkMaxReservableBandwidth Unsigned32,   
      teLinkDescrRowStatus         RowStatus,
      teLinkDescrStorageType       StorageType
  Then I always wonder if we cannot make it easier for people to see which
  objects are indeed in that table. The first object name teLinkDescriptorId
  clealrly is in this table. Then we have one teLinkEncodingType that is not
  so clear. Then we have one that is not as clear as the 1st one, but still
  gives some clue. Then we have 2 that give not clue, then we have 2 more
  that give some clue. How about naming them:
   TeLinkDescriptorEntry ::= SEQUENCE {
      teLinkDescriptorId                Unsigned32,
      teLinkDescrEncodingType           INTEGER,
      teLinkDescrPriority               Unsigned32,
      teLinkDescrMinReservableBandwidth Unsigned32,
      teLinkDescrMaxReservableBandwidth Unsigned32,   
      teLinkDescrRowStatus              RowStatus,
      teLinkDescrStorageType            StorageType
  If you can agree with that, then pls check you other tables. The same 
  issue/concern comes back in a few other tables.
  In fact our mib guidelines document does discuss this issue.

- Your teLinkNotifEnable would be better named: teLinkNotificationsEnabled

- I worry about the number of notifications that can get generated per minute
  or per second? Can you say something about it? Do we need an abject that 
  lets an NMS configure how many notification an agent can send at most per
  minute?

- linkBundleMismatch
  I do not understand the DESCRIPTION clause at all. Specifically, if an error
  is found and the notification is sent, then the 1 second later, the same 
  situation probably still exists. Is another notification sent?
  Should such a problem not be detected when someone creates an antry in
  a table and should create operation not be prevented from succeeding?

- I do not understand the difference between your ...FullCOmpliance and
  ...MonCompliance (by the way, in other MIB modules we have named the
  latter one ...ReadOnlyCompliance). I would think that the ...FullCOmpliance
  one should NOT allow for read-only at all. How can you otherwise claim that
  it is for monitoring AND configuring.

- I also have trouble with:
     OBJECT      teLinkIpAddrType
     SYNTAX      INTEGER { unknown(0), ipv4(1), ipv6(2) }
     MIN-ACCESS  read-only
     DESCRIPTION
         "The dns(16) address type need not be supported.
          The ipv4(1) and ipv6(2) address types need not be
          supported if numbered links are not supported. The
          unknown(0) address type need not be supported if
          unnumbered links are not supported."
   I would rather do something like:
     OBJECT      teLinkIpAddrType
     SYNTAX      INTEGER { unknown(0), ipv4(1), ipv6(2) }
     MIN-ACCESS  read-only
     DESCRIPTION
         "Only ipv4(1) and ipv6(2) address types need to be
          supported for numbered links. For unnumbered links
          unknown (0) needs to be supported."
   In the future other types could be added to InetAddressType TC, and
   so you onbly want to list here what needs to be supported, not 
   (in explicit form) what needs not be supported.

   I wonder if you do not need ipv4z and/or ipv6z.  As long as you have
   evaluated this (with the WG) and decide that you do not need it, then
   fine.

- I have trouble with:
      OBJECT      teLinkRowStatus
      SYNTAX      INTEGER { active(1), notInService(2),
                            createAndGo(4), destroy(6) }
      DESCRIPTION
          "The notReady(3) state need not be supported."

  I think what you want to specify is that createAndWait is not needed.
  Since you allow all objects to be changed in 'active' mode, the 
  notInService may not be needed either. (I am assuming here that I 
  did understand your intent... which I am not sure of). In that case
  something like the following example from RFC3289 would be better:

    OBJECT diffServClfrStatus
    SYNTAX RowStatus { active(1) }
    WRITE-SYNTAX RowStatus { createAndGo(4), destroy(6) }
    DESCRIPTION
       "Support for createAndWait and notInService is not required."

- I have trouble with:
      OBJECT      teLinkStorageType
      SYNTAX      INTEGER { other(1) }
      DESCRIPTION
          "Only other(1) needs to be supported."

  I think I could live with a volatile only, but the 'other' value
  is completely unspecified and seems to make no sense to me at all.
  What is you intention here?

- In security considerations...
  - Do All tables have the same considerations? If so you may want
    to make that explicit.
  - In general, our security ADs probably want some more specificity.
    A statement like "all tables ....." sounds as if you are opting for
    an easy out.
  - On th eread-only objects... are there no issues with bandwith info?
    Or priorities? On protection? 
    would that not reveal info that people don't want to loose?

- Sect 13.1
  Not sure whre [Assigned] is referenced or if it is needed. is it?
  Please note that you must make normative references to all RFCs from
  whihc you IMPORT any objects or TCs or such.
 
- Sect 13.2
  You seem to have kept a lot of old MIB boilerplate references that seem
  no longer needed,.

Thanks,
Bert

---

From: pradeep n <[email-masked]>
Date: Fri, 25 Apr 2003 23:40:24 -0700 (PDT)

Good morning sir, I , Pradeep.N ,completed M.Tech in Computer engg from SJCE college,Mysore.I did my M.Tech Project on diffserv. I know the diffsern very well. If u people give permission to send resume .i will send it inthe next mail.  waiting for earlist positive reply. Pradeep

"Wijnen, Bert (Bert)" <[email-masked]> wrote:In reality, I did review a pre-release of rev 01,
but that rev was pretty much the same as rev 00.

Here is my review:

But.....

- linelegth checker:
$ /bin/checkpage.awk Long line at 770 with 77 chars
Long line at 771 with 76 chars
Long line at 1268 with 73 chars
-: 3 lines longer than 72 characters, max 77

Internet Draft ,
July 2002."

Into

- May I recommend that 

octets contents encoding

- teLinkResourceClass

- I see a few of these:

- linkBundleMismatch

- I have trouble with:
OBJECT teLinkRowStatus

- I have trouble with:
OBJECT teLinkStorageType

- Sect 13.1

- Sect 13.2

Thanks,
Bert 

---------------------------------
Do you Yahoo!?
The New Yahoo! Search - Faster. Easier. Bingo.