[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

AD review of: draft-ietf-snmpconf-diffpolicy-06.txt



Here is my AD review:

1. First, a SMICng compiler (With strict checking) reveals:

  W: f(diffpol.mi2), (20,4) MODULE-IDENTITY diffServConfigMib should 
     have at least one REVISION clause
  W: f(diffpol.mi2), (68,4) Row "diffServConfigEntry" has indexing
     that may create variables with more than 128 sub-ids
   - bw: I think this is a bug in SMICng, cause your limit of 116 seems to
         be limiting it to exact 128 sub-ids
  E: f(diffpol.mi2), (204,12) Item "ifCounterDiscontinuityGroup" should 
     be IMPORTed
  E: f(diffpol.mi2), (207,15) module "DIFFSERV-MIB" may be specified 
     only once
  E: f(diffpol.mi2), (209,12) Item "diffServMIBDataPathGroup" should
     be IMPORTed
  E: f(diffpol.mi2), (209,38) Item "diffServMIBClfrGroup" should 
     be IMPORTed
  E: f(diffpol.mi2), (210,12) Item "diffServMIBClfrElementGroup" should 
     be IMPORTed
  E: f(diffpol.mi2), (210,41) Item "diffServMIBMultiFieldClfrGroup" 
     should be IMPORTed
  E: f(diffpol.mi2), (211,12) Item "diffServMIBActionGroup" should be IMPORTed
  E: f(diffpol.mi2), (211,36) Item "diffServMIBAlgDropGroup" should be IMPORTed
  E: f(diffpol.mi2), (212,12) Item "diffServMIBQGroup" should be IMPORTed
  E: f(diffpol.mi2), (212,31) Item "diffServMIBSchedulerGroup" should be IMPORTed
  E: f(diffpol.mi2), (213,12) Item "diffServMIBMaxRateGroup" should be IMPORTed
  E: f(diffpol.mi2), (213,37) Item "diffServMIBMinRateGroup" should be IMPORTed
  E: f(diffpol.mi2), (214,12) Item "diffServMIBCounterGroup" should be IMPORTed
  E: f(diffpol.mi2), (217,14) Item "diffServMIBMeterGroup" should be IMPORTed
  E: f(diffpol.mi2), (222,14) Item "diffServMIBTBParamGroup" should be IMPORTed
  E: f(diffpol.mi2), (227,14) Item "diffServMIBDscpMarkActGroup" should be IMPORTed
  E: f(diffpol.mi2), (232,14) Item "diffServMIBRandomDropGroup" should be IMPORTed
  E: f(diffpol.mi2), (237,15) Item "diffServDataPathStatus" should be IMPORTed
  E: f(diffpol.mi2), (243,15) Item "diffServClfrStatus" should be IMPORTed
  E: f(diffpol.mi2), (249,15) Item "diffServClfrElementStatus" should be IMPORTed
  E: f(diffpol.mi2), (255,15) Item "diffServMultiFieldClfrAddrType" should be
     IMPORTed
  E: f(diffpol.mi2), (261,15) Item "diffServMultiFieldClfrDstAddr" should be
     IMPORTed
  E: f(diffpol.mi2), (267,15) Item "diffServAlgDropStatus" should be IMPORTed
  E: f(diffpol.mi2), (273,15) Item "diffServRandomDropStatus" should be IMPORTed
  E: f(diffpol.mi2), (279,15) Item "diffServQStatus" should be IMPORTed
  E: f(diffpol.mi2), (285,15) Item "diffServSchedulerStatus" should be IMPORTed
  E: f(diffpol.mi2), (291,15) Item "diffServMinRateStatus" should be IMPORTed
  E: f(diffpol.mi2), (297,15) Item "diffServMaxRateStatus" should be IMPORTed

2. I am also missing the Copyright statement in the DESCRIPTION clause of
   the MODULE-IDENTITY.

3. I am also not sure we need to repeat the whole compliance statements from
   the DIFFSERV MIB. It seems to me it is an exact copy of the 
   diffServMIBFullCompliance, and if indeed so, then I think it is
   sufficient to state (within the DESCRIPTION clause of
   diffServConfigMIBFullCompliance, that the diffServMIBFullCompliance
   must also be met. See 2nd para page 25 of
      draft-ietf-ops-mib-review-guidelines-01.txt

4. When one reads the doc, one would get the impression that one
   MUST undestand the PM-MIB in order to understand this one. Not only
   because of the normative ref, but also reading the text one gets that
   impression. Harrie and Jon had mentioned that this document would
   not need a normative reference to the PM-MIB, but from the text
   it seems to me that it indeed DOES need to, and if not, then it
   does not make clear how this MIB could/should  be used without
   the PM-MIB.

5. I also suspect that the security-considerations section needs to list 
   which tables and objects are security sensitive and why.

6. I also miss the IPR section.

7. There may be other issues that one might find if one does a carefull
   check against:
      draft-ietf-ops-mib-review-guidelines-01.txt 

8. Page 14, 
   Is it really:
      if 
         return roleMatch("Administrator")
   Or is it suppused to be:
      if (roleMatch("Administrator")

   But even with that, I do not think the context is readily understandable
   is it? I think you want to show (possibly with some more complete
   PM-MIB code fragment) how this works and how th various entries
   get created.

   And/Or, show how it can be done without the use of PM-MIB

9. objectdiffServConfigID
       DESCRIPTION
          "A unique id for the per-hop-behavior policy.  The range
          of up to 116 characters is chosen to stay within the SMI
          limit of 128 subidentifiers in an index."
   s/character/octets/
   It is important to distinguis, UTF-8 characters may occupy more than
   one octet per character.


10. StorageType
    Might want to add a DEFVAL ??
    But in any event, you need to say something about write 
    access for permanent entries. See the StorageTypeTC in RFC2579

11. RowStatus
    Need to say which (if any) objects can be changed while a
    row is active. See RowStatus TC in RFC2579

12. Pls remove the IMPORT for InetAddressType, InetAddress,
    they are not used

13. RFC3411 needs to be normatively referenced, you are IMPORTing
    SnmpAdminString from it.

REAL NITS

- page 8, last line of first para
  ".. via a so called the element."  ???

- page 11 2nd para
  "... treatment of 1) is also in the tables."
  Does "1)" refer to section 6.2.1 ?? If so, better make that
  explicit

- Page 13
  I wonder if "DSCP(EF)" is a clear indication of the fact that this
  is entry diffServDscpMarkActDscp.<value for ef> 
  and same for DSCP(AF).

- Pages 12, 13 and 16
  I understand that the values (content) of things like diffServClfrStorage
  and diffServCountActOctets (etc) are irrelevant. But you may want to
  say something about that.

- I'd change
       ::= { mib-2 xxx }  -- Needs to be assigned by before publishing
  into:
       ::= { mib-2 xxx }  -- to be assigned by IANA


Thanks,
Bert