Subject: review of <draft-ietf-ipfc-fabric-element-mib-06.txt>

From: Juergen Schoenwaelder <[email-masked]>
Date: Mon, 13 Dec 1999 16:47:33 +0100

[I apologize for being so late with my comments.]

Here is my review for <draft-ietf-ipfc-fabric-element-mib-06.txt>.
There is still some minor work to be done before the MIB can proceed.
But the document looks in general pretty good now - it has improved
quite a bit. Let me know if there are any comments that are unclear.

Bert, I assumed that the various fibre channel related MIBs will get
rooted directly under mib-2. Please correct me if this is not true.

 1. The text in the 3rd paragraph on page 5 is pretty clear that
    coming and leaving modules do not affect sysUpTime. The last
    sentence in the 5th paragraph says that fcFeModuleLastChange
    indicates when the last change took place. So I expect that
    fcFeModuleLastChange functions as a discontinuity indicator
    for all the counters. I think it would be nice to add a 
    sentence to make this explicit:

    : The object fcFeModuleLastChange acts as the discontinuity
    : indicator for all counter objects in this MIB.

 2. In section 2.1, you write ranges sometimes in () and sometimes
    without (). (This is a minor editorial nit and not really
    important.)

 3. What is the "right" name for this MIB module? I understand that we
    will have more MIB modules for IP over Fibre Channel and hence we
    should carefully pick a naming scheme which can be adopted for
    other IPFC MIBs as well. You currently use FCFABRIC-ELEMENT-MIB.
    You use a very short prefix (FC) which only people familiar with
    Fibre Channel work will be able to understand. Furthermore, you are
    using complete words for the rest of the name. I think it would be
    more useful to use a words at the beginning and to use a short
    acronym on those parts that are Fibre Channel specific. So my
    proposal would be FIBRE-CHANNEL-FE-MIB, which has the same length
    as the current name but will be more readable by the average
    person. Future Fibre Channel MIBs will then use module names like
    FIBRE-CHANNEL-DEVICE-MIB or FIBRE-CHANNEL-DEV-MIB.

 4. Do not import BITS (even if SMICng tells you to do so).

 5. Please import mib-2 since the final version of the MIB will be
    rooted under mib-2.

 6. Make sure the LAST-UPDATED and REVISION dates are updated.

 7. Change the DESCRIPTION clause of the REVISION to:

    :    DESCRIPTION "Initial version, published as RFC XXXX."

    The RFC editor will fill in the RFC number when the document gets
    published.

 8. The current OID structure looks as follows (note that I changed
    comformance to conformance):

    fcFabric
      fcFe
        fcFeMIBObjects
          fcFeConfig
          fcFeOp
          fcFeError
          fcFeAcct
          fcFeCap
        fcFeMIBConformance

    Here is how I think the structure should be:

    fcFeMIB
      fcFeMIBObjects
        fcFeConfig
        fcFeStatus
        fcFeError
        fcFeAccounting
        fcFeCapabilities
      fcFeMIBConformance

    In other words, I propose to change the descriptor of the module
    identity macro to fcFeMIB and to drop the fcFe node. I fixed the
    spelling of "conformance" and I renamed fcFeOp to fcFeStatus and I
    spelled out fcFeAcct and fcFeCap. For the OID value for the module
    identity, please use { mib-2 XXXX }. The RFC editor will fill in
    the right number.

 9. Minor indentation problem in the defintion of FcNameId.

10. Missing "." in the DESCRIPTION of FcAddressId.

11. Use Integer32 instead of INTEGER in the following TCs:
    FcRxDataFieldSize, FcBbCredit, FcphVersion, FcFeNxPortIndex

    (As a general rule: Use INTEGER only for enumerations.)

12. Please remove any ASN.1 comments which say that a group of objects
    is mandatory or optional. Compliance statements are the official
    way to express conformance levels. You can have multiple
    compliance statements. (And in your case, you probably want to
    have two of them - see below). Hence, an ASN.1 comment saying
    something is mandatory or optional is problematic.

13. You should use SnmpAdminString instead of DisplayString in order
    to allow for international character sets. SnmpAdminString is
    defined in (and must be imported from) SNMP-FRAMEWORK-MIB. Also
    remove the comment in the DESCRIPTION of fcFeModuleDescr that the
    value should contain ASCII characters. (The statement was
    redundant anyway since this is what DisplayString requires.)

14. Please change fcFabricName to fcFeFabricName and change
    fcElementName to fcFeElementName. This ensures that you use the
    fcFe and fcFx prefixes throughout the MIB without exceptions.

15. Why is the fcFxPortEntry indexed by 

        INDEX { fcFxPortModuleIndex, fcFxPortIndex }

    and not by

        INDEX { fcFeModuleIndex, fcFxPortIndex } ?

    I do not see why the definition of fcFxPortModuleIndex is needed
    at all.

16. Minor indentation inconsistency in the OID value assignement of
    fcFxPortBbCredit.

17. Suggest to add UNITS "buffers" to fcFxPortBbCredit.

18. Suggest to add UNITS "bytes" to fcFxPortRxBufSize.

19. Suggest to add UNITS "milliseconds" to fcFxPortRatov and fcFxPortEdtov.

20. Suggest to add UNITS "milliseconds" to fcFxPortHoldTime.

21. Suggest to add UNITS "buffers" to fcFxPortBbCreditAvailable.

22. I think fcFxPortOperMode should be read-only instead of read-write.

23. As I wrote earlier, I think the "Operation group" should be named
    "Status group" since this better reflects the information
    contained in this group. This means that some ASN.1 comments must
    be updated and that the fcFxOperTable should be changed into a
    fcFxStatusTable.

24. Suggest to add UNITS "milliseconds" to fcFxPortPhysRttov.

25. Please change fcFxlogiTable to fcFxLoginTable, fcFxlogiEntry to
    fcFxLoginEntry, FcFxlogiEntry to fcFxLoginEntry.

26. The DESCRIPTION of fxFxLoginTable says that it "contains, one
    entry for each FxPort in the Fabric Element, serices parameters
    established from the most recent Fabric Login, explicit or
    implicit". Why has this table a different indexing structure from
    the other tables that have one entry per FxPort? Why can't you
    use INDEX { fcFeModuleIndex, fcFxPortIndex } ?

27. Suggest to add UNITS "buffers" to fcFxPortNxPortBbCredit.

28. Suggest to add a UNITS "bytes" to fcFxPortNxPortRxDataFieldSize. I
    am not sure this is correct since I do not know how to interpret
    the DESCRIPTION of this object. What is meant by "binary value"
    here?

29. Are the accounting table really augments? I mean, do you really
    want to have for every FxPort an entry in all three accounting
    tables? Or is it possible that some FxPorts only have entries in
    the C1 accounting table while other FxPorts only have entries in
    the C2 accounting table? In the second case, do not use AUGMENTS.
    Instead repeat the INDEX clause from the fcFxPortTable.

30. Is fcFxPortC1ConnTime really a Counter32? The description talks
    about a cumulative time. A counter only counts events and is not a
    type for measuring time intervals.

31. Minor indentation problem in the definition of the
    FcFxPortC2AcctEntry SEQUENCE.

32. Suggest to add UNITS "buffers" to fcFxPortCapBbCreditMax and
    fcFxPortCapBbCreditMin.

33. Suggest to add UNITS "bytes" to fcFxPortCapRxDataFieldSizeMax and
    fcFxPortCapRxDataFieldSizeMin.

34. Suggest to add UNITS "milliseconds" to fcFxPortCapHoldTimeMax and
    fcFxPortCapHoldTimeMin.

35. You currently have a single compliance statement, which requires
    to implement the config and the status group. It is surprising
    that this compliance statement does not require the implementation
    of the error group. I also suggest to add a second compliance
    statement for those implementations that implement all MIB groups.

    Furthermore, you should check whether write access to read-write
    objects is required to be conformant. (You can formally express in
    the compliance statement that write access is not needed.) Not
    sure this is relevant for this module since there are only a few
    writable objects and they seem to be pretty easy to implement.

    Also, some of the tables may be conditionally mandatory. For
    example, a device which does not support class 3 frames can not be
    expected to implement the class 3 accounting table. In this case,
    you probably have to break the OBJECT-GROUPs into smaller pieces.

    Below is a proposal to start from:

     -- compliance statements
     fcFeMIBMininumCompliance   MODULE-COMPLIANCE
         STATUS   current
         DESCRIPTION   
            "The minimum compliance statement for SNMP entities
             which implement the FIBRE-CHANNEL-FE-MIB."
         MODULE  -- this module
         MANDATORY-GROUPS { fcFeConfigGroup, fcFeStatusGroup, fcFeErrorGroup }
      ::= { fcFeMIBCompliances 1 }

     fcFeMIBFullCompliance   MODULE-COMPLIANCE
         STATUS   current
         DESCRIPTION   
            "The full compliance statement for SNMP entities
             which implement the FIBRE-CHANNEL-FE-MIB."
         MODULE  -- this module
         MANDATORY-GROUPS { fcFeConfigGroup, fcFeStatusGroup, fcFeErrorGroup 
	                    fcFeAccountingGroup, fcFeCapabilitiesGroup }
      ::= { fcFeMIBCompliances 2 }

/js

-- 
Juergen Schoenwaelder      Technical University Braunschweig
<[email-masked]>  Dept. Operating Systems & Computer Networks
Phone: +49 531 391 3289    Bueltenweg 74/75, 38106 Braunschweig, Germany
Fax:   +49 531 391 5936    <URL:http://www.ibr.cs.tu-bs.de/~schoenw/>

---

From: Kha Sin Teow <[email-masked]>
Date: Mon, 13 Dec 1999 11:47:44 -0800

thanks Juergen, for the detailed review of the document.

I will revise the draft next week.
Do you want me to send the revised one to you for a quick review again
before I submit to the IETF draft depository?

Will be in touch.

Juergen Schoenwaelder wrote:

> ...

[email-masked]

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Tue, 14 Dec 1999 10:09:41 +0100

>>>>> Kha Sin Teow writes:

Kha> I will revise the draft next week.  Do you want me to send the
Kha> revised one to you for a quick review again before I submit to
Kha> the IETF draft depository?

I think we can turn things around faster if you send me the new
revision directly. So the next version which goes to the ID repository
should be the final version.

/js

---

From: Kha Sin Teow <[email-masked]>
Date: Tue, 14 Dec 1999 08:42:06 -0800

> should be the final version.

that's great!  I'll work on revising the document end of this week or early
next week.

--
----------------------------------------------------------------------
Kha Sin Teow                            [email-masked]
Brocade Communications Systems          Tel: 408-487-8180
1901 Guadalupe Parkway                  Fax: 408-487-8090
San Jose, CA 95131                      http://www.Brocade.Com
----------------------------------------------------------------------