Subject: RE: [Ips] FW: Initial review of http://www.ietf.org/internet-draf ts/draft-i

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 19 Mar 2004 19:57:30 +0100

Sorry for later response/reaction

Comments inline, where I feel I need to add something

Thanks,
Bert 

> -----Original Message-----
> From: Keith McCloghrie [mailto:[email-masked]]
> Sent: vrijdag 6 februari 2004 23:55
> To: [email-masked]
> Cc: [email-masked]; [email-masked]; [email-masked]
> Subject: Re: [Ips] FW: Initial review of
> http://www.ietf.org/internet-drafts/draft-i
> 
> 
> Bert/Mike,
> 
> Thanks for the review.
> 
> Below, please find :
> 
> a) my respones to the comments, and
> b) the changes that I plan to make as a result of the review.
> 
> Thanks,
> Keith.
> 
> > Thanks Mike for the first MIB Doctor review.
> > 
> > authors/wg members,
> > Pls copy Mike on any responses, cause I suspect he is not
> > subscribed to IPS mailinglist (Mike tell us if you are,
> > in which case people do not specifically have to copy you).
> > 
> > Thanks,
> > Bert 
> > 
> > -----Original Message-----
> > From: Michael MacFaden [mailto:[email-masked]]
> > Sent: woensdag 4 februari 2004 7:02
> > To: Wijnen, Bert (Bert)
> > Cc: Anurag Maunder
> > Subject: Initial review of
> > http://www.ietf.org/internet-drafts/draft-ietf-ips-fcmgmt-mib-04.txt
> > 
> > 
> > Hi Bert,
> > 
> > My review of the fiber channel management MIB module:
> > 
> > http://www.macfaden.com/ietf/fc-mgmt-mib-4-reviewed.txt
> > 
> > Regards,
> > Mike
> > 
> 
> --------------------------------------------------------------
> ----------------
> 
>  
> > Feb 3, 2004 MIB DOCTOR Review: 
> http://www.ietf.org/internet-drafts/draft-ietf-ips-fcmgmt-mib-04.txt
> > 
> > Performed by Michael R. MacFaden, Kazeon Systems, Inc
> > according to draft-ietf-ops-mib-review-guidelines-02.txt
> > 
> > A. Documentation Guidelines 
> >    3.1 o MIB boilerplate section       -> present
> >            current version?            -> YES
> >    o Narrative sections                -> present
> >            Scope described?            -> YES
> >            Spelling?                   -> No obvious mistakes 
> >            Module structure described? -> YES
> >            Dependent on other modules? -> YES
> >              If so, which ones?        -> IF-MIB
> >            Relates to other modules?   -> YES 
> >              If so, which ones?        -> 
> HOST-RESOURCES-MIB, ENTITY-MIB, 
> >            References ([XXX])?         -> Yes, they are 
> correct and resolve properly
> > 
> >            Reviewer Comments:
> >            1. "high performance capability" isn't further 
> defined so first 
> >               sentence is so much fluff.
> 
> This sentence which appears in the first sentence of the 
> "Short Intro to
> Fibre Channel" section, was copied from the Introduction section of
> Fibre Channel standards documents.  The intent is to give a high-level
> indication of what Fibre Channel is aimed at, and to phrase 
> it so as to
> remain valid in the future as and when the specific bandwidth 
> supported
> by Fibre Channel is higher than it is today.  It is too early in the
> into to be definitive.  Thus, it isn't "fluff".
> 
> >            2. Text describing the MIB module's structure 
> (Sect 4.1-11) is better than the GROUP DESCRIPTION
> >               clauses in the compliance section.
> >            3. IF-MIB description was superbly done.
> >    o Definitions section present        -> present
> >        Written to SMIv2 standards?   -> YES
> >        Issues with compilation?      -> YES
> >    o Intellectual Property section                   -> present
> >           Matches current boilerplate?               -> YES
> >           Any proprietary rights claimed?            -> NO
> >    o References section                              -> present
> >       Matches current boilerplate (2578-80,3410)?    -> YES 
> >       Normative/informative sections separate?       -> YES
> >       MIB modules listed in IMPORTS included?        -> 
> >    o Security section                                -> present
> >        Matches current boilerplate?                  -> YES
> >        Specific Read/write objects listed?           -> YES
> >        Security risk spelled out?                    -> YES
> >        Specific Read only objects listed?       
> > 
> >       Reviewer Comments:
> >          There was only one specific security risk 
> described. More could be "spelled out."
> >          For instance, setting fcmSwitchDomainId to an 
> invalid value could lead to 
> >          denial of service in some configurations.
>              
> No matter how much one writes in this section, it is always true that 
> "more could be spelled out".
> 
Keith, pls don't read it so litrrally. I guess Mike did see room
for improvement. And he in fact gives an example as to what can be improved.

> >    o IANA Considerations  section                   -> not present
> >        Was a namespace defined?                     -> NO
> >           
> >    o Copyright Notices section                      -> present
> >        Correct year specified?                      -> NO
> >        Copyright found in MODULE-IDENTITY?          -> NO
>       
> Noted.
>  
and will be fixed I assume?

> >    o Definitions section detail 
> > 
> >    Here are the diagnostics from certain mib syntax checkers:
> > 
> > Diagnostics  from Version 0.4.2 smilint -l9 
> > C:\local\smi\mibs\ietf>smilint -m -s -l 9 ./FC-MGMT-MIB
> > ./FC-MGMT-MIB:2213: [5] {group-unref} warning: current 
> group `fcmLinkBasicGroup' is not referenced in this module
> 
> OK, I will add a GROUP clause to "make it clear that the 
> optional status is intended".
> 
Thanks

> > ./FC-MGMT-MIB:62: [5] {type-without-format} warning: type 
> `DomainIdOrZero' has no format specification
> > ./FC-MGMT-MIB:94: [5] {type-without-format} warning: type 
> `FcBbCredit' has no format specification
> > ./FC-MGMT-MIB:106: [5] {type-without-format} warning: type 
> `FcDataFieldSize' has no format specification
> > ./FC-MGMT-MIB:171: [5] {type-without-format} warning: type 
> `PhysicalIndexOrZero' has no format specification
> > ./FC-MGMT-MIB:181: [5] {type-without-format} warning: type 
> `HrSWInstalledIndexOrZero' has no format specification
> > ./FC-MGMT-MIB:192: [5] {type-without-format} warning: type 
> `MilliSeconds' has no format specification
> > ./FC-MGMT-MIB:198: [5] {type-without-format} warning: type 
> `MicroSeconds' has no format specification
> 
> It seems to me like the compiler has a bug here, since all of these are, or
> have a TC as their syntax and for all but two of them, the TC is defined 
> with a range, e.g., DomainIdOrZero is defined as:
> 
>   DomainIdOrZero ::= TEXTUAL-CONVENTION
>       STATUS current
>       DESCRIPTION
>             "The Domain Id (of a FC switch), or zero if the no Domain Id
>             has been assigned."
>       SYNTAX  Integer32 (0..239)
> 
> The two exceptions are MilliSeconds and MicroSeconds which are both
> Unsigned32 and the inherent (0..4294967295) is correct for both.
>  
I think the warning is cause by the fact that there is no DISPLAY-HINT.
Can you add those?

> > Diagnostics from MIB Smithy 2.4.2
> > 
> > 
> [FC-MGMT-MIB:fcmgmtCompliance:FC-MGMT-MIB:fcmInstanceSoftwareI
> ndex] : warning : SYNTAX in variation and OBJECT-TYPE differ 
> in type names 'Integer32' and 'HrSWInstalledIndexOrZero'.
> > 
> [FC-MGMT-MIB:fcmgmtCompliance:FC-MGMT-MIB:fcmInstancePhysicalI
> ndex] : warning : SYNTAX in variation and OBJECT-TYPE differ 
> in type names 'Integer32' and 'PhysicalIndexOrZero'.
> 
> Yes, but both HrSWInstalledIndexOrZero and PhysicalIndexOrZero are TCs
> which have Integer32 as their underlying syntax.
> 
And no change is needed. I think the Smithy should be fixed.

> > [FC-MGMT-MIB:fcmLinkEnd2UnitType] : warning : BITS 
> named-number list values should start at zero.
> > [FC-MGMT-MIB:fcmFLoginClassesAgreed] : warning : BITS 
> named-number list values should start at zero.
> 
> Another bug in the compiler.  Both objects have a TC as their syntax where
> the TC has a named-number list which does start at zero.
> 
Agreed

> > [FC-MGMT-MIB:fcmISPortEntry] : warning : All INDEX objects 
> in this table are auxiliary (i.e., from other tables).
> > [FC-MGMT-MIB:fcmFxPortEntry] : warning : All INDEX objects 
> in this table are auxiliary (i.e., from other tables).
> 
> Eh?  The compiler has another bug, since:
> 
> a) auxiliary does NOT mean "i.e., from other tables", rather RFC 2758
> section 7.7 defines "auxiliary" as follows:
> 
>    Objects which are both specified in the INDEX clause of a conceptual
>    row and also columnar objects of the same conceptual row are termed
>    auxiliary objects. 
> 
> Thus, the error message as stated is actually wrong.
> 
I believe so too

> b) It is common practice for media-specific MIBs which extend the
> IF-MIB to define a table which is INDEX-ed only by ifIndex.
> 
Agreed and no issue I think

> 
> > [FC-MGMT-MIB:fcmPortFcOperClass] : warning : BITS 
> named-number list values should start at zero.
> > [FC-MGMT-MIB:fcmPortFcCapClass] : warning : BITS 
> named-number list values should start at zero.
> > [FC-MGMT-MIB:fcmPortEntry] : warning : All INDEX objects in 
> this table are auxiliary (i.e., from other tables).
> > [FC-MGMT-MIB:fcmInstanceFunctions] : warning : BITS 
> named-number list values should start at zero.
> > Validation complete - 0 error(s), 10 warning(s)
>  
> See above.
> 
> >   Reviewer Comments:  
> >    1. The TC's could possibly go into a separate TC 
> document for fiber channel: FcNameIdOrZero, DomainIdOrZero, 
> FcAddressId
> >       as it is mentioned there might be multiple Fiber 
> channel mib modules in the future.
> 
> While your "could" and "possibly" are correct, but we have already
> written several of these follow-on FC MIB modules, and they are
> IMPORT-ing TCs from this MIB without any problems.  So, this 
> would be a disruptive change for no benefit.
> 
Keith, but the current spec locks all other MIB modules to this
one. I.e. the otehrs cannot advance unless this one also advances.
If you put TCs in a separate module then you do not have that issue.
Now it can still be changed (rather easily), later it becomes more
difficult. But it is not a MUST change

> >    2. FcAddressId doesn't have an *OrZero version, which 
> might be needed at a later date.
> 
> It's syntax is "OCTET STRING (SIZE(0 | 3))" and the zero-length value
> is already used (see fcmLinkEnd2FcAddressId).
> 
You migth want to add text to the DESCRITPION clause that explains the
special meaning of zero.

I think what Mike noticed is an inconsistency, because some of your other
TCs do have an xxxOrZero. Consistency is (I think ) a good thing.
But a again not a MUST change.

> >    3. FcPortType enumeration "dynamic(3),  -- determined 
> dynamically"  seems redundant, prefer comment point to underlying spec
> 
> No, it's not redundant because "dynamic" could also mean that 
> the value
> is constantly changing, and saying "determined dynamically" 
> removes that possibility.
> 
You might want to enhance the description somewhat then.
And as Mike says, a ptr to the spec might be helpful too

> >    4. PhysicalIndexOrZero, HrSWInstalledIndexOrZero are 
> >       very useful and generally applicable to many mib modules.
> >       They look strangly wrong being defined here and 
> >       should be defined in another TC, like Entity-MIB, or some 
> >       common TC-MIB.
> 
> But they aren't, and thus, the best that this MIB module can do is to
> define the TC's itself.
> 
Could you rename them to RcPhysicalIndexOrZero and FcHrSWInstalledIndexOrZero
so that they will not conflict if Entity MIB does define them later.

But... SURPRISE (maybe), pls take a look at

   draft-ietf-entmib-v3-04.txt

and is on my AD plate for review!! So at least that one can be
imported from ENTITY-MIB.

> >    5. MicroSeconds, MilliSeconds should also be in some 
> generic tc-mib...
> 
> But they aren't, and thus, the best that this MIB module can do is to
> define the TC's itself.
> 
Well, the SSPM-MIB (draft-ietf-rmonmib-sspm-mib-11.txt) which is in
IETF Last Call DOES have a SspmMicroSeconds TC.
I made them prefix with Sspm.. because I do foresee a future generic
TC MIB for these things. So if you want to keep them in this MIB
module I strongly recommend to name it FcMicroSeconds and FcMilliSeconds.

> >    6. fcmInstanceIndex DESCRIPTION is problematic for me. 
> Either it stays static or it doesn't. 
> >       Having something "desireable" is worthless to those 
> writing mangement applications, they have to assume worst case always.
> 
> Not worthless, because if agent implementors heed the advice, 
> then mgmt
> apps can coded to check whether the index values have been retained
> over the last restart, and only if not, do they need to re-synchronize
> their database.  That is, it is an optimization for the common case of
> index values being retained, but the fallback still works in the case 
> (hopefully less common) when all knowledge of index values is lost.
> 
So would it make sense to phrase it with a SHOULD ??

> >       There are problems with the description. Sentences 
> start without capitalization.
> 
> "Sentences" ??   I see only one such sentence-break, for which the fix
> is to delete the the period before "but" (because it's bad grammar to
> start a sentence with "But".)
> 
> >       fcmInstanceTable has very critical data. Would like 
> to have a LastChange entry per row to know when values were changed or
> >         a "configuration change" notification if lastChange 
> object is too much overhead.
> 
> But the data is not, in general, both critial and dynamic.  
> Specifically,
> the only things in this table that change are: 
> - the FabricId if it's based on the WWN of the Principal switch;
>     -- a notification of a Principal switch change is being 
> proposed as
>        an extension to this MIB.
> - the read-write objects (but only when an app changes them);
>     -- changes in these values are not critical to the 
> operation of the
>        network, and the synchronization of mgmt apps should 
> not have to
>        rely on notifications from agents for such non-critical data.
> - the Status object,
>     -- to monitor this object for changes, set an RMON 
> threshold on it.
>        Note that the typical situation is to have just one instance;
>        even if there are multiple instances, there will not be many.
>        Having an RMON threshold will be more effective than having a
>        LastChange object.
> 
> >    7. fcmInstanceWwn, but does it matter which WWN is 
> selected? Will managment stations expect one vendor 
> implemetnation over another?
> 
> The ability to have multiple instances is defined as a flexibility to
> allow for many/any different choices by vendors.  Vendors are free to
> choose to have one, or more, instances using whatever 
> methodology they wish.
> Thus, we cannot prescribe how the WWN is selected.
> 
> This flexibility is necessary because otherwise there is a 
> genuine risk
> that a vendor will implement a set of Fibre Channel 
> interfaces in a manner
> which cannot be represented by this MIB.
> 
> >       Wouldn't it be the same as fcmSwitchDomainId if 
> fcmSwitchPrincipal is True?
> 
> It could be but it needn't be, and the circumstance you ask about is a
> corner case which for example never applies to HBAs.
> 
> >    8. fcmInstanceStatus could use an associated object to 
> get timestamp of the last state change
> 
> While implementation feedback might tell us that, I think it's too
> soon to be sure.  So, let's save this comment until we update 
> the MIB from
> Proposed to Draft Standard.
> 
> >    9. fcmInstanceStatus doesn't seem to be implementable in 
> a consistent way across vendors. Needs better definition
> >     ideally this state is defined in the underlying 
> technology specifications.
> 
> On the contrary, a) there is no one underlying technology, and b) the
> underlying technology specifications are the subject of future MIBs,
> not this one.  This object serves as a generic summary of the 
> underlying
> specifics (e.g., somewhat like ifOperStatus does for an interface.)
> 
> >    10. fcmInstanceTextName/fcmInstanceDescr, are there any 
> DEFAULT values for these? Suspect vendor implementations will
> >       provide something. Better to define the intended 
> purpose of these objects better.
> >             "A textual name for this management instance 
> and the Fibre
> >             Channel entity/entities that it is managing."
> >       might be 
> >             "A textual name for this management instance 
> and the Fibre
> >             Channel entity/entities that it is managing set 
> by the operator or to a default string "XXXX" by the device."
> 
> "XXXX" can not be specified in this MIB without restricting 
> the necessary
> flexibility, anything that could be specified in this MIB would not be
> specific enough.  The whole idea of this object is to be the 
> name specified
> by the operator/NM-app, to have a default value changes the semantics.
> 
> >    11. fcmPortAdminType - Assuming a valid value change 
> can't be done for some device resource specific case, how can
> >                           the error be reported? A generic 
> snmp error to the set is necessary but not sufficient I think.
> 
> Why would it not be sufficient ??
> 
cause genErr is also used for other (unspecified) errors.
Maybe this could be spelled out that the best would be to sent an
inconsistentValue error?

> >    12. Not sure why these object's syntax wasn't a TC? They 
> will probably be added to in time as well.
> >             fcmPortCapProtocols
> >             fcmPortTransmitterType 
> >             fcmPortConnectorType -- this should probably 
> follow the MAU-MIB convention or we need to standardize 'JackType'
> >                                     in a new TC mib as 
> there is now overlap...
> 
> Actually, not for the latter two because they are not generic 
> definitions.
> Rather they are the specific enumerations listed for the function in
> T11's GS-3 speciofication.  Sicne a future revision of the GS-3 spec
> could revise them individually, they cannot be defined as common TCs.
> The first one could, but I don't see any rule that it violates which
> would justify a change at this time.
> 
A change is justified as much as no change. My personal opinion.
Is it a MSUT change? no it is not.

> >    13. fcmPortStatsTable doesn't have an explicitly 
> specified discontinuity object -- is ifXTable entry the one 
> that applies?
> 
> Yes.
> 
Then you want to state that in DESCRIPTION clause(s)

> >                Also, the counters described here should 
> have REFERNCE clauses to counters defined in the underlying 
> technology 
> >                specs. While the descriptions are generally 
> good, they do not cover all possible cases when 
> >                one should be incremented or not -- that 
> leads to incompatible implementations. 
> 
> Many of them are not specifically defined in any Fibre Channel
> specification.  If any are ambiguous, please suggest alternative
> wording.  Otherwise, I suggest we wait for implementation feedback.
>                 
> >    14. fcmPortLcStatsTable doesn't have split counters?  Is 
> a SNMPv1 agent then really useful when running high speed interfaces?
> 
> No, but at the time (several years ago now) when this was discussed
> in the WG, many Storage vendors only had SNMPv1 agents, and as a
> result, this table was defined at the explicit direction of the WG.
> 
I.e. to force them to SNMPv3? FIne by me!

> >             DESCRIPTION
> >             "A list of Counter32-based statistics which are 
> a shadow of
> >             the Counter64 statistics in the fcmPortStatsTable, for
> >             systems which do not support Counter64."
> >       I suggest the text mention it should "shadow" the 
> lower 32 bits?
>  
> If that isn't what "shadow" means, then I don't know what it 
> does mean, and we shouldn't use the term at all.
> 
I prefer if you spell out they contain the low order 32 bits.

> >    15. If errors are not so common an occurance across 
> instances, the fcmPortErrorsTable might benefit from a 
> *LastChange object
> >        so not all objects have to be polled to determine 
> any error state change. Alternately provide a summary counter that
> >        is a simple sum of the number of errors. That should 
> allow for better scaling of this table for mgmt polling using 
> SNMP/UDP.
> 
> The summary counter for many of these is already available: 
> ifInErrors.
>        
> >    16. Error table should have REFERENCE clauses to point 
> out where in the underlying spec these counters are defined. 
> >        This appears to be done, but is inconsistent. 
> fcmPortLinkFailures has a REFERENCE but fcmPortLinkResets wasn't?
>  
> As I recall, it is consistent; specifically, all those for which
> there is an appropriate section to reference have a REFERENCE clause.
> 
> >    17. fcmFxPortTable needs a discontinutity object. if 
> fcmPortOperType is changed from Fx to some other then back within
> >        a given polling interval, the data collected from 
> this table will be incorrect.
> 
> No, because there are no Counter-based objects in this table.
> 
> >    18. Objects in fcmFxPortTable, fcmFLoginTable could use 
> REFERENCE clauses.
> 
> Sigh !!  I will look again to see what I can find.
> 
> >    19. fcmFLoginNxPortIndex doesn't specify what behaviors 
> should be across SNMP agent reset or system reboots like
> >        fcmInstanceIndex is required to do. So I'm a bit 
> concerned what the indexing looks like for all the tables on
> >        system reset or agent reset.
> 
> I will add a sentence saying that after a value of 
> fcmFLoginNxPortIndex
> is assigned to a particular Nx_Port, it can be re-used when and only
> when it is assigned to the same Nx_Port, or, after a reset of 
> the value
> of the relevant instance of ifCounterDiscontinuityTime.
> 
> >    20. fcmLinkEnd2AgentAddress OBJECT-TYPE is  SYNTAX      
> SnmpAdminString, not InetAddressType? Why?
> 
> Because the value is received from the otrher end of the 
> link, and if the
> other end supplies a bogus value,m then that bpgus value 
> needs to be stored
> in the MIB.
> 
So in that case it can be stored with InetAddressType "unknown" and
the value that you received in the InetAddress, no?

So I do not see a reason to NOT use InetAddress(Type) combo.
> >    21. 
> > End of Report

An additional remark from me:

I strongly recommend to rename DomainIdOrZero to FcDomainIdOrZero.
The current definition of the TC is certainly not a generic DomainId
definition is it?

Bert
> 
> --------------------------------------------------------------
> ----------------
> 
> 
> - fix the Copyright dates, and insert a Copyright statement in
>   the MODULE-IDENTITY.
> 
> - add
>        GROUP fcmLinkBasicGroup
>        DESCRIPTION
>             "This group is optional."
> 
> - delete the period in  "... across restarts.  (but note, it is ..."
> 
> - add an explicit statement that discontinuities in the counters in
>   the fcmPortStatsTable can only occur when the value of the relevant
>   isntance of ifCounterDiscontinuityTime is zero.
> 
> - look again to see what references I can find to put in 
> REFERENCE clauses
>   of objects in fcmFxPortTable, fcmFLoginTable.
> 
> - add a sentence saying that after a value of fcmFLoginNxPortIndex
>   is assigned to a particular Nx_Port, it can be re-used when and only
>   when it is assigned to the same Nx_Port, or, after a reset 
> of the value
>   of the relevant instance of ifCounterDiscontinuityTime.
> 

_______________________________________________
Ips mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/ips

---

From: Keith McCloghrie <[email-masked]>
Date: Tue, 23 Mar 2004 11:48:51 -0800 (PST)

Bert,

> Comments inline, where I feel I need to add something

and my responses.

You can find the updated MIB at:

  ftp://ftpeng.cisco.com/ftp/kzm/pub/draft-ietf-ips-fcmgmt-mib-05.txt

I'll submit it as an updated I-D if I don't receive any comments in the
next few days.

Thanks,
Keith.

> and will be fixed I assume?

Right; it was on the list of fixes that I previously enclosed.

> I think the warning is cause by the fact that there is no DISPLAY-HINT.

I can't because, as far as I know, there is no standard format for
displaying these.  Anyway, RFC 2579 says that DISPLAY-HINT is optional.
So, this is another bug in that compiler.  Why is the IETF's MIB review
process relying upon a buggy compiler ??

> difficult. But it is not a MUST change

Since they are "follow-on FC MIB modules", it is entirely appropriate
that they not advance until this one does, and anyway most of them
would still be dependent on this MIB, even if the TC's were in a
separate document.

> But a again not a MUST change.

Because the TC is not currently imported by any other MIB, I've made the
change that you ask for (but not the addition of another TC that Mike
asked for).
 
> And as Mike says, a ptr to the spec might be helpful too

I've added a couple of words and moved it inside the DESCRIPTION
instead of being an ASN.1 comment.

> so that they will not conflict if Entity MIB does define them later.

There will be no conflict.  First, the possibility of a descriptor
clash is ever-present in practically every deployment because of
Enterprise MIBs.  Second, MIB compilers have to cope with it, and they
do for the most part, although HP OpenView's compiler has had the same
bug for the last ~10 years, and most often shows up because of
OwnerString being defined as an OCTET STRING in IF-MIB and as a
DisplayString in RMON-MIB; if you delete one of the two definitions or
change them to both have the same syntax, OpenView stops complaining.

> imported from ENTITY-MIB.

First, we don't want to risk any further delays.  (In the update that I've
just done, I changed the date from February 2003 to March 2004.  In other
words, we have waited 13 months for the benefit of the minor changes that
I'm making now :-(.)  So, let's not import from draft-ietf-entmib-v3-04.txt.

Second, as you may have noticed all(*) the FC-specific TC's in this MIB
are already prefixed with "Fc", and none of the generic TC's are
prefixed with "Fc".  I want to keep it that way, and since neither of
PhysicalIndexOrZero or HrSWInstalledIndexOrZero are FC-specific, I've
just deleted them.  (* -- actually, all but one because I made a
mistake with DomainIdOrZero.)

> module I strongly recommend to name it FcMicroSeconds and FcMilliSeconds.

Again, naming them FcMilliseconds and FcMicroseconds would give the
reader the wromng impression.  These are NOT Fibre Channel units; they are
generic units. Fibre Channel's milliseconds/microseconds are NO different
from any other technology's milliseconds/microseconds.  Nevertheless,
because of the comment, I've deleted the two TC's.

> So would it make sense to phrase it with a SHOULD ??
 
It wouldn't make sense to say a system SHOULD retain the values if all of
its non-volatile memory is replaced, i.e., to say that it should when we
know there are circumstances when it can't !!

> Maybe this could be spelled out that the best would be to sent an

This comment applies to every read-write/read-create in this and every
other MIB.  To explain when to return which error status for a SetRequest
requires most of pages 20-22 of RFC 3416 to be included in the DESCRIPTION
of each such MIB object, and that doesn't make sense.

It seems to me that the thing which prompted this comment was the
second sentence in the DESCRIPTION:

            "The administratively desired type of this port.  Each port
            will typically only be able to support a subset of these types."

Is it really necessary for me to remove the second sentence ??

> Then you want to state that in DESCRIPTION clause(s)

I've added this to all tables which have CounterNn objects.

> I prefer if you spell out they contain the low order 32 bits.

I removed the phrase containing "shadow".

> the value that you received in the InetAddress, no?

No.  The GS-3 spec gives, as an example, the use of a URL in this
string, with the URL being for an SNMP, or HTTP or XML agent. 
I've added some words to the DESCRIPTION stating this.

> The current definition of the TC is certainly not a generic DomainId

Agreed.  DomainIdOrZero is FC-specific, and so it was my mistake to
call it DomainIdOrZero.

Keith.