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

From: Kha Sin Teow <[email-masked]>
Date: Mon, 17 Jan 2000 10:47:39 -0800

Hi,

I got the comments from Juergen on the latest draft of Fabric Element MIB.
He had 33 editorial comments, and 2 that are technical.

I believe the editorial comments are straightforward to deal with
and I would accept the changes.

I would like to discuss the two technical comments.

> 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.
>
To the best of my recollection, this was defined to accumulate the number of
bytes (including IDLEs and other primitive sequences) associated with
the class 1 connection. since one could figure out the byte rate of the
connection, one could therefore derive the cumulative time.
In retrospect, this seems like an awkward way to keep the time.
I propose to change this definition to count the cumulative time
in units of milliseconds.

> 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 }
>
I tend to accept what Juergen suggested.
However, the issue of supporting "write" access needs to be discussed.
I believed the original intent was to make the "write" access optional.
and that's what I'll define the conformance statement in the next revision.

For your convenience, Jurgen's comments are attached below.

Juergen Schoenwaelder wrote:

> [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/>

--
----------------------------------------------------------------------
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
----------------------------------------------------------------------