Subject: Re: IESG quality review for draft-ietf-ipfc-fabric-element-mib-04

From: Kha Sin Teow <[email-masked]>
Date: Thu, 24 Jun 1999 23:51:07 -0700

Juergen Schoenwaelder wrote:

> >>>>> Bert Wijnen writes:
>
> Bert> Juergen, thans for your commitment to do a quality review of
> Bert> this MIB within the next couple of weeks.  Pls send your
> Bert> comments to the author and cc: the WG chair and ADs as listed
> Bert> above.
>
> Below is my review for <draft-ietf-ipfc-fabric-element-mib-04.txt>.
> Some points repeat several times - that is why the list looks quite
> long. Some of the comments listed here have already been raised by
> Bert Wijnen - you probably have already adressed them.  Feel free to
> contact me if you have any questions.

Hi Mr. Schoenwaelder,

Thanks for your review and comments.

I revised the document and will submit a new draft tomorrow.
As the editor of the document, I took the liberty to incorporate what I consider
trivial editorial
changes (no major technical change).

There are other suggestions that I agree in principle, but does not have time to
completely
incorporate them due to time constraint.
I will tag my response to those comments with "R1".

There are some that I thought may affect implementations and therefore I would
like to get the WG to
discuss them before I change them. I will tag my response to those specific
comments with "R2".

>  1) I am confused by the statement "version 2" since this is going to
>     be published as Proposed Standard (see Bert's comment).

It's deleted.

>  2) You should use the MIB boilerplate available from www.ops.ietf.org.

done.

>  3) There is a reference to [12] in section 3 which does not exist in
>     the references section.

corrected.

>  4) Section 3.1 starts "> From" - the "> " got probably inserted by a
>     mail system.

yes.

>  5) We have learned that it is useful to carefully check that limits
>     (e.g. range or size restrictions) defined in MIB modules have good
>     architectural reason. Note that you can't change any limits once
>     the MIB has been published. First example: Is there an
>     architectural reason for restricting fcFeModuleCapacity to 256
>     (see section 3.1)?

agree. This particular limit is removed.

>  6) Section 3.1 says that "modules may come and go without causing a
>     management reset." Please make sure that all counter objects
>     related to a module clearly indicate the discontinuity indicator
>     in the DESCRIPTION clause.

agree.

>  7) Is there an architectural reason to limit fcFeModuleFxPortCapacity
>     to 256 (section 3.1)?

no - the limit is removed.

>  8) Is there an architectural reason to limit NxPort to 126 (section
>     3.1)?

yes - this is defined in FC-AL.

>  9) I suggest to remove "(manager, agent, or proxy agent)" from the
>     first sentence in section 3.2.

done.

> 10) I have some question wrt. the figure at the end of section
>     3.1. What is [i], [n] and [L]?

corrected with additional explanation.

> 11) Section 4.1 says that sysServices will have an integer value "2"
>     or higher. I do not really like this description. Perhaps it is
>     sufficient to say that Fibre Channel is a datalink layer protocol
>     (layer 2).

> 12) In section 4.2 you refer to the "interface group". I think you
>     should instead refer to the IF-MIB and add a reference. The latest
>     version is defined in RFC 2233.
>
> 13) I think section 4.3 can be removed. If you want to keep this
>     statement, you should refer to RFC 1907 instead of the "system
>     group".
>
> 14) You should also change the title of section 4. This section
>     describes the relationship to other relevant MIBs. (I generally
>     prefer to avoid the term MIB-II since this is ambiguous. RFC 1213
>     has been split in many smaller modules and its just a matter of
>     time until it will go to historic.)

I removed this section  to avoid confusions.

> 15) The OID value for the MODULE-IDENTITY macro is invalid.

I assumed that the value will be assigned as part of the IESG approval
process.Does the IPFC WG need to apply for this assigned value separately from
IANA?

> 16) You should turn all ASN.1 type definitions into SMIv2
>     TEXTUAL-CONVENTIONS.

done.

> 17) Should FabricName not be named FcFabricName? Shoud FcNameId be
>     renamed to FcName? Note that it is not allowed in SMIv2 to derive
>     new TEXTUAL-CONVENTIONS from existing TEXTUAL-CONVENTIONS.

collapsed all into FcNameId.

> 18) What about IPv6 addresses and FcNameId?

Unfortunately, the definition of FC World_wide name does not include IPv6.It's an
issue that is outside the scope of this document.

> 19) Is there an architectural reason for restricting FcAddressId to
>     24 bits?

> 20) Is there an architectural reason for the range of FcRxDataFieldSize?
>
> 21) Is there an architectural reason for the range of FcBbCredit?
>
> 22) Is there an architectural reason for the range of FcphVersion?
>     Should this type be named FcPhVersion for consistency?

yes - these are defined in FC-PH.

> 23) Is BITS not a more appropriate type for FcCosCap?

This is an R2.

> 24) Is there an architectural reason for the range of
>     FcFeModuleCapacity and FcFeFxPortCapacity?

No - the limits have been removed.

> 25) Is there an architectural reason for the range of FcFeModuleIndex,
>     FcFeFxPortIndex and FcFeNxPortIndex?

No for FcFeModuleIndex and FcFeFxPortIndex;yes for FcFeNxPortIndex (see response
to Comment 8).

> 26) The comment above fcFabricName defines compliance
>     requirements. You should put these into formal compliance
>     definitions. (Comments should in general not carry any important
>     information in MIB modules.)

This is an R1 - I will incorporate a Conformance Statement in the next rev.

> 27) Index elements must generally be not-accessible.

agree - corrected.

> 28) fcFeModuleDescr is of type DisplayString. Note that this implies
>     that it contains printable ASCII characters (which means that this
>     statement in the DESCRIPTION clause is not needed). In order to
>     facilitate internationalization, you should seriously consider to
>     use SnmpAdminString instead of DisplayString.

This is an R2.

> 29) I am wondering whether the type for fcFeModuleLastChange should be
>     TimeStamp rather than TimeTicks.

This is an R2.

> 30) What about changing fcFxConfTable to fcFxPortTable? I think using
>     the fcFxPort prefix consistently in this table helps to understand
>     the structure of the MIB.

This is an R2.

> 31) There is text in several places about obsoleted variables. I
>     suggest to remove this text since this MIB is just going to enter
>     the standards track (and hence there should not be any obsolete
>     objects).

All except fcFPortFlogiTable and FcFxPortOperTable have been removed.I believe
fcFPortFlogiTable and FcFxPortOperTable are an R2.

> 32) What about adding UNITS clauses to fcFxPortRxBufSize,
>     fcFxPortRatov, fcFxPortEdtov and fcFxPortHoldTime?

This is an R1.

> 33) The comment above the fcFxPortOperTable defines conformance
>     requirements. This should be moved into appropriate compliance
>     definitions.

This is an R1.

> 34) fcFxPortOperTable should augment fcFxPortTable if there will
>     always be exactly one entry in the fcFxPortOperTable for each
>     entry in the fcFxPortTable. Otherwise, you should at least reuse
>     the index components from the fcFxPortTable rather than defining
>     similar obejct types for the fcFxPortOperTable.

This is an R2.

> 35) All columns in a conceptual table should have the same prefix in
>     their names. (This may lead to longer names than you have now. But
>     it helps to understand the MIB structure.)

This is an R1.

> 36) The registration of the columns in fcFxPortOperTable is not
>     contiguous due to some of those "obsolete" objects. You may want
>     to change that by removing the obsolete objects and filling up the
>     gaps.

same as response to Comment 31 above.

> 37) Is the fcFxPortPhysTable another augmentation of fcFxPortTable? If
>     there is a sparse relationship, then use the index elements of the
>     fcFxPortTable in the fcFxPortPhysTable.

This is an R2.

> 38) The MIB allows to set fcFxPortAdminMode to `unknown'. Does this
>     make sense?

corrected.

> 39) fcFxPortPhysOperStatus says that "other values may be used to
>     indicate online/testing diagnostic for failed tests." This is no
>     good idea since different implementations will use the same named
>     number with different semantics. This causes interoperability
>     problems on the manager side.

agree - corrected.

> 40) Should fcFxPortPhysLastChange have type TimeStamp rather than
>     TimeTicks?

This is an R2.

> 41) What about adding a UNITS clause to fcFxPortPhysRttov?
>
> 42) What about changing fcFxlogiTable to fcFxLoginTable and using the
>     fcFxLogin prefix consistently?

these are R1.

> 43) You should reuse fcFxPortModuleIndex and fcFxPortIndex rather than
>     defining fcFxlogiModuleIndex and fcFxlogiFxPortIndex in the INDEX
>     clause of fcFxlogiEntry.

this is an R2.

> 44) In some DESCRIPTION clauses in the fcFxlogiTable, you refer to
>     FxPort where I thought you actually mean an NxPort.

Not sure why there is confusion. For now, I treat it as an R1.

> 45) Should there be UNITS clauses for fcFxPortNxPortBbCredit and
>     fcFxPortNxPortRxDataFieldSize?

This is an R1

> 46) The last sentence in the DESCRIPTION of
>     fcFxPortNxPortRxDataFieldSize can be removed since the range
>     limits are already defined in the type definition.

agree - corrected.

> 47) I am wondering whether fcFxPortIntermixSuppAgreed,
>     fcFxPortStackedConnModeAgreed, fcFxPortClass2SeqDelivAgreed,
>     fcFxPortClass3SeqDelivAgreed should not be merged into a single
>     BITS type. This can probably all be merged into
>     fcFxPortCosSuppAgreed?

This is an R2.

> 48) The DESCRIPTION of fcFxPortNxPortName says that the value has some
>     default if no NxPort is attached to the FxPort. I wondering how
>     this can happen since the table in indexed by an NxPort number...

agree - corrected.

> 49) I suggest to change the logic of some sentences. For example, the
>     DESCRIPTION of fcFxPortConnectedNxPort says:
>
>         If the value of this object is '000000'H, this FxPort is not
>         engaged in a connection.
>
>     I think its better to first name the condition and afterwards
>     describe which value applies:
>
>         If this FxPort is not engaged in a connection, then the value
>         of this object is '000000'H.
>
>     This applies as well to several other sentences.

agree - corrected.

> 50) I found the DESCRIPTION of fcFxPortBbCreditModel confusing. The
>     text more or less reads like a description of the FcBbCreditModel
>     type rather than the fcFxPortBbCreditModel obejct type.

I removed the additional explanation.

> 51) The text above fcFxPortErrorTable contains a conformance
>     requirement which should be made explicit in a compliance
>     statement.

this is an R1.

> 52) The fcFxPortErrorTable is probably another table augmentation or
>     you should at least reuse the INDEX components of the
>     fcFxPortTable.

agree - R2

> 53) The counters in the fcFxPortErrorTable should have a clear
>     indication which object serves as a discontinuity indicator.

This is an R2.

> 54) The comments above the fcFxPortC1AcctTable define conformance
>     requirements. This should be moved into appropriate compliance
>     definitions.

This is an R1

> 55) The accounting tables seem to either augment the port table or
>     they have a sparse augmentation relationship. In the later case,
>     use the index object of the port table.

this is an R2.

> 56) What you call accounting groups look more like statistic groups in
>     the IETF MIB module slang. Is accounting the term used in the
>     fibre channel standards? This would be a good reason to use the
>     term in the MIB as well. Otherwise, I would probably suggest to
>     talk about statistics rather than accounting throughout the MIB.

agree - this is an R2.

> 57) The description of fcFxPortC1ConnTime says that "the amount of
>     time of each connection is counted in octets from after a
>     connect-request has been accepted...". I am confused how you count
>     time in octets. Sounds more like fcFxPortC1ConnOctets to me.

this is an R2.

> 54) The comment above the fcFxPortC2AcctTable defines conformance
>     requirements. This should be moved into appropriate compliance
>     definitions.

this is an R1.

> 58) The accounting tables have similar objects (e.g. *InFrames,
>     *OutFrames). It would be nice if similar objects would be at
>     similar positions in the table. (In other words, put similar
>     objects at the beginning of the SEQUENCE.)

agree - this is an R2.

> 59) The comment above the fcFxPortCapTable defines conformance
>     requirements. This should be moved into appropriate compliance
>     definitions.

this is an R2.

> 60) Check the table indexing of fcFxPortCapTable.

not sure what you mean?

> 61) fcFxPortCapBbCreditMax, fcFxPortCapBbCreditMin,
>     fcFxPortCapRxDataFieldSizeMax, fcFxPortCapRxDataFieldSizeMin,
>     fcFxPortCapHoldTimeMax, fcFxPortCapHoldTimeMin
>     should probably have UNITS clauses.

agree - this is an R1.

> 62) You should only have one References sections and references should
>     have a unique identification.

I changed that into three sub-sections for IETF References, Approved
ANSI/NCITSReferences and ANSI/NCITS References Under Development respectively.
References now have a unique identification.

> 63) A security considerations section must be written. See the OPS web
>     server http://www.ops.ietf.org for some guidelines.

done.

> 64) You need a copyright and probably also an IPR section.

done.

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

Again, thanks for your  comments.

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