AD review of: draft-ietf-ipcdn-bpiplus-mib-12.txt - part 1

Sorry, it took too long  to finally get this done.

Not sure this document is ready for IETF Last Call.
Actaully I think it is not.

This is part 1. Mainly serious things.
I may have more nits/admin stuff later.

Here are my findings.

More or less serious:
1. I see various read-write and/or read-create objects and I do
   not see any text in the DESCRIPTION clauses (non STorageType objects)
   that tell me what the expected behaviour is w.r.t. persistency of such
   objects. SO what happens after a restart/reboot?

2. I see a number of ZeroBasedCOunter32 objects that have text aka:
   (for example docsBpi2CmAuthentInfos)
            DESCRIPTION
                 "The value of this object is the count of times the CM
            has transmitted an Authentication Information message,
            since reboot."
   It is OK to tell us that a ZeroBasedCOunter32 object must start with zero
   at (re-)boot time or at row creation.
   But from that point on, a ZeroBasedCounter32 behaves exactly the same
   as a Counter32, and so it is incorrect to say "count of X since reboot"
   because the Counter32 may have wrapped!. Possibly this is not gonna
   happen in practice, but literally the claim is incorrect.

3. For these ZeroBasedCounter32 objects, I also see no word about a possible
   discontinuity timer. Why not. Can there NEVER be a discontinuity? Of if
   there is one does that mean ALL counters experience a discontionuity?
   The latter is what you basically state (or cause) by not pointing to a
   specific discontinuity timer. Because then by default it is sysUpTime, and
   so that means that when you DO experience a discontinuity, then you MUST 
   reset sysUpTime and that means that a discontinuity for EVERYONE (object)
   that assumes the default. If such is intended, then fine, but it would be
   good to then state that sysUpTime is the discontinuity timer.

4. I see that for some objects you speak about a "null string" or "NULL string".
   The base data type for such objects is OCTET STRING. In all those cases
   I suspect (but I am not sure) that you mean the zero-length octet string.
   Otherwise I do not understand what "null string" means.
   Pls explain and fix.

5. For docsBpi2CmtsAuthCmExpiresOld I see:
            Note: For CMs running in BPI mode, implementation of this
            object is optional and MAY vary."
   Mmm... that sounds like a MODULE-COMPLIANCE aspect and I would rather
   see such things in MODULE-COMPLIANCE and not in object DESCRIPTION clauses.

6. I see:
      -- Note: the following object has been obsoleted

      docsBpi2CmtsAuthCmReset  OBJECT-TYPE
           SYNTAX    INTEGER   {
                               noResetRequested(1),
                               invalidateAuth(2),
                               sendAuthInvalid(3),
                               invalidateTeks(4)
                               }
           MAX-ACCESS     read-write
           STATUS         current

    So the --Note: is out of sync with the actual status!?
    What is it? If it IS obsoleted, then status should sya so,
    And DESCRIPTION clause should explain why it was obsoleted.
    And the ASN.1 comment line then of course is no longer needed.

7. I see:
      docsBpi2CmtsAuthCACertIndexPtr    OBJECT-TYPE
            SYNTAX         Integer32 (0..10000)
   And find that a strange limit (range). And nowhere, not even in the 
   docsBpi2CmtsCACertTable do I see an explanation why that range makes
   sense (assuming that it does).

8. When I see:
            docsBpi2CmtsIpMulticastAddressType      InetAddressType,
            docsBpi2CmtsIpMulticastAddress          InetAddress,
            docsBpi2CmtsIpMulticastMaskType         InetAddressType,
            docsBpi2CmtsIpMulticastMask             InetAddress,
   I wonder if (in the same row) docsBpi2CmtsIpMulticastMaskType will ever
   have a different value then docsBpi2CmtsIpMulticastAddressType !??
   It seems to me that should NOT be allowed, cause otherwise I am not
   sure how the ANDing of the Mask is going to work/happen.
   So the next question then is why you do not do:
            docsBpi2CmtsIpMulticastAddressType      InetAddressType,
            docsBpi2CmtsIpMulticastAddress          InetAddress,
            docsBpi2CmtsIpMulticastMask             InetAddress,
   And let docsBpi2CmtsIpMulticastAddressType be the discriminator for both
   InetAddresses. One less object, and less change for error/conflict.

   But thinking even further, Possibly the best thing to do is to use
            docsBpi2CmtsIpMulticastAddressType      InetAddressType,
            docsBpi2CmtsIpMulticastAddress          InetAddress,
            docsBpi2CmtsIpMulticastPrefixLength     InetAddressPrefixLength,
   Are not such masks always setup that they basically specify a prefix length?
   If so, then this is the way to do it with the TCs from INET-ADDRESS-MIB.

9. I see various uses of InetAddress as for example here:
       docsBpi2CmtsIpMulticastAddress          OBJECT-TYPE
            SYNTAX         InetAddress
            MAX-ACCESS     read-create
            STATUS         current
            DESCRIPTION
                 "This object represents the IP multicast address
            to be mapped, in conjunction with
            docsBpi2CmtsIpMulticastMask."
   The TC for InetAddress (in RFC3291 or its follow on) clearly state that
   you MUST specify which InetAddressType controls the format of this object
   as per DESCRIPTION from InetAddress TC:
         An InetAddress value is always interpreted within the context
         of an InetAddressType value. Every usage of the InetAddress
         textual convention is required to specify the InetAddressType
         object which provides the context. ...

10. I see:
      docsBpi2CmtsIpMulticastMapControl  OBJECT-TYPE
           SYNTAX         RowStatus
           MAX-ACCESS     read-create
           STATUS         current
           DESCRIPTION
                "This object controls and reflects the IP multicast
           address mapping entry.  There is no restriction on the
           ability to change values in this row while the row is
           active.  Inactive rows need not be timed out."
    Mmm... that "need not be timed out" seems in conflict with the RowStatus
    TC DESCRIPTION clause in RFC2579. Can you explain why this is? 

    Also, a RowSTatus object MUST specify in its DESCRIPTION clause under
    which conditions 
    - the row can be activated
    - which columns (if any) can bve changed while in the active state.
    I am missing the first.

11. You specify:
       --
       -- The BPI+ MIB Conformance Statements (with a placeholder for
       -- notifications)
       --

       docsBpi2Notification     OBJECT IDENTIFIER
            ::= { docsBpi2MIB 2 }
       docsBpi2Conformance OBJECT IDENTIFIER
            ::= { docsBpi2MIB 3 }
       docsBpi2Compliances OBJECT IDENTIFIER
            ::= { docsBpi2Conformance 1 }
       docsBpi2Groups      OBJECT IDENTIFIER
            ::= { docsBpi2Conformance 2 }

   Why not be (more) consistent with other MIB modules and follow the
   suggested OID subtrees as per MIB guidelines, 
   draft-ietf-ops-mib-review-guidelines-03.txt, appendix D:
        xxxMIB
        |
        +-- xxxNotifications(0)
        +-- xxxObjects(1)
        +-- xxxConformance(2)
            |
            +-- xxxCompliances(1)
            +-- xxxGroups(2)
   This is not mandatiory, but consistency is always useful/helpful


Nits/administrativia:

1. The RFC editor wants all references to have at least one citation
   in the document. ALso for the normative references to RFCs from
   which you import. See MIB review guidelines, sect 3.5

   You need to add a citation somehwere for [RFC3411], [RFC2021],
   [RFC3291], [RFC2670]

Thanks,
Bert