Subject: [manet] AD review of draft-ietf-manet-nhdp-mib-11

From: "Adrian Farrel" <[email-masked]>
Date: Sun, 11 Mar 2012 13:33:32 -0000

Hi,

Many apologies for a delayed AD review of this document. I love MIB
modules deeply, but I did want to clear out the other MANET documents
from the pipe before working on this one.

As usual, the purpose of my review is to catch and clean up issues that
might show later in the process, thereby saving community and IESG
review effort, and giving the document a smoother ride through the
system and a greater likelihood of approval.

This is a substantial document and is generally solid. The number of
comments reflects the fact that writing MIB modules is a real back-
breaker. I think it is important to note that I did not have any
concerns about the structure of the module or the tables (except one
small suggestion about moving the TCs into a separate module). Most
of the comments are at the level of MIB nits.

All comments are, of course, up for discussion and debate.

I think we need a new revision at this point. Then we can go to MIB
Doctor review and IETF last call at the same time.

Many thanks,
Adrian

---

5.1.3.1

   The majority of critical events occur when NHDP is enabled on a
   router, at which time the symmetric neighbors and two-hop neighbors
   of the NHDP router are discovered.

I think we can clarify this because there are currently two
interpretations:

- when NHDP is operational (enabled)
- when NHDP is initiated (enabled or the first time)

>From context, I think you mean...

   The majority of critical events occur when NHDP is first enabled on a
   router, at which time the symmetric neighbors and two-hop neighbors
   of the NHDP router are discovered.

---

5.1.3.1

   To avoid unnecessary notifications, a router should not
   originate expected notifications until a certain time interval has
   elapsed, which is to be predefined by the network manager.

1. Is this "SHOULD NOT"?
2. Is there a recommended default for such a timer value? I think you
   should give one if you can.

---

5.1.3.2

   Appropriate values for the window time and upper bound are to be
   selected by the network manager and depend on the deployment of the
   MANET.

I believe you here, but I would welcome some more guidance for the
implementer and the deployer. Is there anything you can add?

---

5.1.3.3

OLD
   Similar to the according mechanism in [RFC4750], only one
   notification is sent per event.
NEW
   Similar to the mechanism in [RFC4750], only one notification is sent
   per event.
END

---

Section 6

   This section specifies the relationship of the MIB modules contained

s/modules/module/

---

You have included Section 6.3 which is perfect.
In the light of this, there is a body of opinion amongst "MIB experts"
that citation notation should not be used in the body of the MIB
module. This is because the module will be ripped from the document
for compilation and passed around as a stand-alone piece of text.

Thus, just take the square brackets out form the comments in the
IMPORT clauses.

Make the same change to any Description and Reference clauses.

---

Don't forget to make any necessary changes to the CONTACT-INFO clause.

---

The copyright date in the MODULE-IDENTITY clause is a little out-of-
date.

---

For sound reasons, revisions of the MIB module posted in Internet-Drafts
are not considered as "Revisions". That is, per the boilerplate, it is
inappropriate to use Internet-Drafts as reference material or to cite
them other than as "work in progress.

That means that the first revision of the MIB Module is the one that
gets published in the RFC.

Can you tidy the Revision clauses so that there is only one, and it
applies to the RFC-that-will-be. The RFC Editor will fix up the date on
publication.

---

NeighborIfIndex

I'm not saying what you have is wrong, but why did you decide to not
assign this TC the Syntax InterfaceIndex?

---

For nhdpInterfaceTable I think it is necessary to say that when the
corresponding entry with ifIndex value is deleted from the Interface
Table, the entry in this table is automatically deleted.

---

nhdpIfIndex has Syntax InterfaceIndexOrZero, but the Description says
"The ifIndex for this interface."

Note that according to RFC 2863, ifIndex has Syntax InterfaceIndex.

So you need to decide to either:
- change the Syntax of nhdpIfIndex
or
- add an explanation of what it means to have a zero value

---

nhdpIfStatus

I *think* it is more normal to write:

      DEFVAL { false(2) }

---

nhdpInitialPending

Given the constraint conditions described for this object, I don't
think it is appropriate to have a Default clause. That is, if the
constraining objects are not defaulted for whatever reason, then this
object cannot be allowed to default.

This is correctly reflected in you making the object read-only, and we
should note that there is no value in assigning a Default for a read-
only object.

---

Finding nhdpIfRowStatus and seeing the text talk about row creation, I
wondered why none of the objects in the row under the control of this
object have Max-Access read-create. What you have is legal, but it means
that the row is created with all the defaults in place and then can be
modified if the management application wants to.

Since there is no equivalent of an adminStatus, this means that for a
while the row (and associated protocol elements) will operate with the
default values rather than the values the management application wanted
to set.

---

The second paragraph of the description of nhdpLibLocalIfSetTable begins
"It consists of..."  This is mildly ambiguous because the previous
paragraph ends with a sentence about nhdpIfIndex.

---

nhdpLibLocalIfSetIpAddrType should contain a comment about the valid
values since I assume you don't support the full set defined for
InetAddressType. You can also constrain the interpretation of
nhdpLibLocalIfSetIpAddr according to nhdpLibLocalIfSetIpAddrType.

You would write...

   nhdpLibLocalIfSetIpAddrType  OBJECT-TYPE
      SYNTAX      InetAddressType
      MAX-ACCESS  read-write
      STATUS      current
      DESCRIPTION
         "The type of the nhdpLibLocalIfSetIpAddr
          in the InetAddress MIB [RFC4001].

          Only the values unknown(0), ipv4(1), and
          ipv6(2) are supported."
      REFERENCE
         "[RFC6130]."
   ::= { nhdpLibLocalIfSetEntry 1 }

   nhdpLibLocalIfSetIpAddr  OBJECT-TYPE
      SYNTAX      InetAddress
      MAX-ACCESS  read-write
      STATUS      current
      DESCRIPTION
         "nhdpLibLocalIfSetAddr is an
          address of an interface of
          this router.

          This object is interpretted according to
          the setting of nhdpLibLocalIfSetIpAddrType."
      REFERENCE
         "[RFC6130]."

This will also need to show up in the conformance statements. You
write something like...

OBJECT nhdpLibLocalIfSetIpAddrType
  SYNTAX       InetAddressType { unknown(0), ipv4(1), ipv6(2) }
  MIN-ACCESS   read-only
  DESCRIPTION  "Only unknown(0), ipv4(1), and ipv6(2) support
                is required."

And similarly...

OBJECT nhdpLibLocalIfSetIpAddr
  SYNTAX      InetAddress (SIZE(0|4|16))
  MIN-ACCESS  read-only
  DESCRIPTION "An implementation is only required to support
               address sizes of:
                 0 for address type unknown(0)
                 4 for address type ipv4(1)
                 16 for address type ipv6(2)."

Lastly, is the value zero allowed for nhdpLibLocalIfSetIpAddr? If so
you will need some text in its DESCRIPTION clause like...

     If this object is set to 0, the value of
     nhdpLibLocalIfSetIpAddrType must be set to
     unknown(0)."

You would need to do this for all uses of InetAddress[Type]

---

You need to do a general search and replace s/MIB/MIB module/
There is only one MIB and this MIB module forms part of it.
For example...

nhdpStateObjGrp    OBJECT IDENTIFIER ::= { nhdpObjects 2 }

   -- Two new constructs have been defined in this MIB for
   -- indexing into the following
   -- tables and indexing into other tables in other MIBs.

---

Question...

nhdpStateObjGrp    OBJECT IDENTIFIER ::= { nhdpObjects 2 }

   -- Two new constructs have been defined in this MIB for
   -- indexing into the following
   -- tables and indexing into other tables in other MIBs.

1. This text seems to come very late in the module. How about
   relocating to be next to the definition of the "constructs"?

2. I think s/constructs/Textual Conventions/

3. If it is clear that you will use these TCs to index other
   MIB modules, you should seriously consider putting them in
   their own module. You can keep this new module in this
   document, but making it a separate module makes imports from
   other modules much more simple.

---

nhdpUpTime could use a display hint clause.

---

   nhdpInterfaceStateTable  OBJECT-TYPE
      SYNTAX      SEQUENCE OF NhdpInterfaceStateEntry
      MAX-ACCESS  not-accessible
      STATUS      current
      DESCRIPTION
         "nhdpInterfaceStateTable lists state information
          related to specific interfaces of this NHDP router.
          The ifIndex is from the interfaces group
          defined in the Interfaces Group MIB.

Unless I am mistaken, you are not using ifIndex, but ndhpIfIndex.
So you should say...

          The value of ndhpIfIndex is an ifIndex from the
          interfaces group defined in the Interfaces Group
          MIB.

Similarly nhdpInterfaceStateEntry (which is missing a REFERENCE clause.

---

There is nothing wrong, IMHO, with using TimeTicks for nhdpUpTime and
nhdpIfStateUpTime. However, this places a burden on the implementation
rather than the agent.

A common approach is to grab the sysUpTime value when the instance was
initialised, and to only report that. An agent that is interested in
the amount of time that the instance has been up, can read sysUpTime
and do the calculation itself.

If you grab sysUpTime, you use the Textual Convention TimeStamp which
ironically has the Syntax TimeTicks. The difference is that it is frozen
rather than your usage which is rolling.

---

As I understand it, nhdpIibLinkSetLTime is a time in the future. I'm not
sure, butI don't think you are allowed to use TimeStamp like this
because it is supposed to report on an event that has already happened.
This may be a bit pettifogging and we should maybe make a list of
questions to ask the MIB Doctor.

---

nhdpInterfacePerfTable etc.

For you to think about depending on the interface speeds you think you
are supporting. The requirements on the use of 32-bit and 64-bit
counters copied from RFC 2863 are as follows:

   For interfaces that operate at 20,000,000 (20 million) bits per
   second or less, 32-bit byte and packet counters MUST be supported.
   For interfaces that operate faster than 20,000,000 bits/second,
   and slower than 650,000,000 bits/second, 32-bit packet counters
   MUST be supported and 64-bit octet counters MUST be supported.
   For interfaces that operate at 650,000,000 bits/second or faster,
   64-bit packet counters AND 64-bit octet counters MUST be
   supported.

---

   nhdpSetNotification OBJECT-TYPE
          SYNTAX       OCTET STRING (SIZE(4))
          MAX-ACCESS   read-write
          STATUS       current
          DESCRIPTION
             "A 4-octet string serving as a bit map for
             the notification events defined by the NHDP
             notifications. This object is used to enable
             and disable specific NHDP notifications where
             a 1 in the bit field represents enabled. The
             right-most bit (least significant) represents
             notification 0.

             This object is persistent and when written
             the entity SHOULD save the change to
             non-volatile storage.
             "
           ::= { nhdpNotificationsControl 1 }

I find the use of Octet String in this way a bit icky. It is, of course,
possible to represent the whole MIB module using Octet Strings, but we
don't. Did you consider and reject the BITS construct? It seems neater
and more explicit.

I also spent some time trying to work out which notification is
"notification 0" because the first notification listed is
{ nhdpNotificationsObjects 1 }

---

Should the ChangeThreshold and ChangeWindow family of objects have
DEFVALs?

---

nhdpNbrState, nhdp2HopNbrState, and nhdpIfState are (correctly)
read-only. They shouldn't have DEFVALs defined because they report what
they report.

You might want to add to the Description...

If the state of foo is unknown, an implementation should return bar(n)

Or you might want to define specific "unknown" values to handle this,

---

Really good job on the Security Considerations section!

---

I would fold Sections 10 and 11 together since "Contributors" in an
RFC are effectively at "Author" status.