Subject: RE: [imss] Re: AD review of: draft-ietf-imss-fc-fspf-mib-02.txt

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Thu, 6 Apr 2006 00:17:30 +0200

Keit/Dan,

I am OK with the new revision 2.

Sect 7 is still redundant with back matter!

But.. doc is ready for IETF LC I think.

Bert

> -----Original Message-----
> From: Wijnen, Bert (Bert) [mailto:[email-masked]]
> Sent: Wednesday, March 08, 2006 21:17
> To: Keith McCloghrie
> Cc: [email-masked]; [email-masked]; [email-masked]; [email-masked];
> [email-masked]; [email-masked]
> Subject: RE: [imss] Re: AD review of: 
> draft-ietf-imss-fc-fspf-mib-01.txt
> 
> 
> Inline
> 
> > -----Original Message-----
> > From: Keith McCloghrie [mailto:[email-masked]]
> > Sent: Tuesday, March 07, 2006 16:24
> > To: [email-masked]
> > Cc: [email-masked]; [email-masked]; [email-masked]; [email-masked];
> > [email-masked]; [email-masked]; [email-masked]
> > Subject: [imss] Re: AD review of: draft-ietf-imss-fc-fspf-mib-01.txt
> > 
> > 
> > Hi Bert,
> >  
> > > Summary:
> > > 
> > > looks good. I would be OK with doing IETF Last Call and 
> consider these
> > > comments as initial IETF Last Call comments. At the other 
> hand, IETF
> > > LC right before/during an IETF will not get many people to pay
> > > attention to this. Maybe you rather do a new rev first?
> >  
> > I'll do a new rev first, but it won't now get posted until 
> > after the IETF.
> > 
> 
> I think the repository opens on Monday of IETF week already.
> 
> > > more serious questions/issues:
> > > 
> > > SMICng tells me:
> > >   E: f(t11fcfspf.mi2), (915,54) Index item 
> "t11FspfLinkIndex" must be
> > >      defined with syntax that includes a range
> > >   W: f(t11fcfspf.mi2), (1007,19) Variable "ifIndex" in 
> notification
> > >      "t11FspfNbrStateChangNotify" is an index for a table
> > > 
> > > The warning is OK/acceptable as far as I am concerned.
> > > The Error would probably be best fixed.
> >  
> > SMICng is confused:
> > 
> > - t11FspfLinkIndex is defined as Unsigned32, which is defined with a
> >   range of (0..4294967295).  So, it does have a range (but 
> see below).
> > 
> seem my comment below
> 
> > - ifIndex is read-only.  RFC 2578 says (in section 7.3):
> > 
> >     The value "not-accessible" indicates an auxiliary 
> object (see Section
> >     7.7).  The value "accessible-for-notify" indicates an 
> object which is
> >     accessible only via a notification (e.g., snmpTrapOID [5]).
> > 
> >     These values are ordered, from least to greatest:  
> "not-accessible",
> >     "accessible-for-notify", "read-only", "read-write", 
> "read-create".
> > 
> >   Since "read-only" is greater than "accessible-for-notify", it is
> >   both readable and accessible via a notification.  Indeed, consider
> >   the situation described in bullet (2) of section 7.7 of 
> RFC 2578 --
> >   the only way to have a notification include any 
> information from a row
> >   of a table for which all of the row's objects are also 
> specified in
> >   its INDEX clause, is for it to include an object from the 
> INDEX clause.
> > 
> 
> Understood, but we do have the desire to not include INDEX items.
> For ifIndex, one could have used ifDescr.
> I would further think that t11FspfIfNbrDomainId also already includes 
> the ifIndex in its INDEX part of the OID.
> But I agree with you that this is not a big issue, as I said above:
> 
>    The warning is OK/acceptable as far as I am concerned.
> 
> 
> > > SMILINT tells me:
> > > 
> > > C:\smi\mibs\work>smilint -m -s -l 6 ./T11-FC-FSPF-MIB
> > > ./T11-FC-FSPF-MIB:212: [5] {identifier-case-match} warning: 
> > identifier `t11FspfA
> > > RegionNum' differs from `T11FspfARegionNum' only in case
> > > ./T11-FC-FSPF-MIB:61: [6] {previous-definition} info: 
> > previous definition of `T1
> > > 1FspfARegionNum'
> > > ./T11-FC-FSPF-MIB:824: [5] {identifier-case-match} warning: 
> > identifier `t11FspfL
> > > srType' differs from `T11FspfLsrType' only in case
> > > ./T11-FC-FSPF-MIB:71: [6] {previous-definition} info: 
> > previous definition of `T1
> > > 1FspfLsrType'
> > > ./T11-FC-FSPF-MIB:964: [5] {identifier-case-match} warning: 
> > identifier `t11FspfL
> > > inkType' differs from `T11FspfLinkType' only in case
> > > ./T11-FC-FSPF-MIB:89: [6] {previous-definition} info: 
> > previous definition of `T1
> > > 1FspfLinkType'
> > > ./T11-FC-FSPF-MIB:61: [5] {type-without-format} warning: 
> > type `T11FspfARegionNum
> > > ' has no format specification
> > > ./T11-FC-FSPF-MIB:71: [5] {type-without-format} warning: 
> > type `T11FspfLsrType' h
> > > as no format specification
> > > ./T11-FC-FSPF-MIB:89: [5] {type-without-format} warning: 
> > type `T11FspfLinkType'
> > > has no format specification
> > > ./T11-FC-FSPF-MIB:129: [5] {type-without-format} warning: 
> > type `T11FspfLastCreat
> > > ionTime' has no format specification
> > > 
> > > The identifier-case-match are acceptable. They may confuse 
> > people though.
> > > 
> > > The type-without-format warnings are for your TCs.
> > > You may want to add a SIPLAY-HINT
> >  
> > Both section 3.1 of RFC 2579 and section 4.6.3 of RFC 4181 say that
> > the DISPLAY-HINT clause is optional/need not be present.
> >  
> 
> As I said: you MAY want to add a DISPLAY-HINT (well I see I 
> misspelled it)
> so I will appreciate if you do, but I won't get sleepless 
> nights if you don't.
> 
> > > - sect 5 states:
> > >    protocol.  (Note that there are no definitions in this 
> MIB module of
> > >    "managed actions" which can be invoked via SNMP.)
> > >   So what does that mean for t11FspfSetToDefault, which 
> to me seems to
> > >   be an action that can be invoked via SNMP??
> > >   Same for t11FspfIfSetToDefault
> >  
> > It depends on how one defines "action", and you're right 
> that the term
> > "managed action" is not well-defined.  That sentence was 
> added based on
> > a question that arose in T11.  The term "managed action" 
> was intended
> > to mean a setRequest which caused a switch to send out a 
> command in one
> > of the T11-defined protocols; for comparison, see some of 
> the objects
> > defined in draft-ietf-imss-fc-fam-mib-03.txt (e.g., t11FamRestart).
> > Since that sentence is adding confusion, I will delete it.
> > 
> Thanks
> 
> > > - redundant objects??
> > >   I do not see what the difference is between the follow two
> > >   (well, except SYNTAX). Is there any? If so, can that be made
> > >   clear in the DESCRIPTION clauses? If not, can one be removed?
> > > 
> > >   t11FspfLsrs OBJECT-TYPE
> > >     SYNTAX      Gauge32
> > >     MAX-ACCESS  read-only
> > >     STATUS      current
> > >     DESCRIPTION
> > >            "The current number of entries for this Fabric in the
> > >            t11FspfLsrTable."
> > >     ::= { t11FspfEntry 19 }
> > > 
> > >   t11FspfLsrNumber  OBJECT-TYPE
> > >     SYNTAX      Unsigned32 (0..2147483647)
> > >     MAX-ACCESS  read-only
> > >     STATUS      current
> > >     DESCRIPTION
> > >            "The current number of rows for this fabric in the
> > >            t11FspfLsrTable."
> > >     ::= { t11FspfEntry 23 }
> >  
> > Thanks for spotting that.  I will remove one of them.
> > 
> Good
> 
> > > - t11FspfStorageType
> > >   Which writeable objects must be writable for a 'permanent' row?
> >  
> > Since I already succumbed in adding the sentence that you're looking
> > for to t11FspfIfStorageType, I will (so as to be 
> consistent) add it to
> > t11FspfStorageType also.
> > 
> Mmm.. my comment was/is mainly based on text from RFC2579 (on 
> of yours):
> StorageType ::= TEXTUAL-CONVENTION
>     STATUS       current
>     DESCRIPTION
>             "Describes the memory realization of a conceptual row.  A
>             row which is volatile(2) is lost upon reboot.  A row which
>             is either nonVolatile(3), permanent(4) or readOnly(5), is
>             backed up by stable storage.  A row which is permanent(4)
>             can be changed but not deleted.  A row which is 
> readOnly(5)
>             cannot be changed nor deleted.
> 
>             If the value of an object with this syntax is either
>             permanent(4) or readOnly(5), it cannot be written.
>             Conversely, if the value is either other(1), 
> volatile(2) or
>             nonVolatile(3), it cannot be modified to be 
> permanent(4) or
>             readOnly(5).  (All illegal modifications result in a
>             'wrongValue' error.)
> 
>             Every usage of this textual convention is required to
>             specify the columnar objects which a permanent(4) row must
>             at a minimum allow to be writable."
> 
> So that last sentence to me seems to require it.
> 
> 
> > However, the requirement at the top of page 23 in RFC 4181 is bogus
> > because: a) a requirement which says that an "object must be 
> > writeable"
> > under some particular circumstance is for the minimum 
> requried access,
> > i.e., MIN-ACCESS, and belongs in MODULE-COMPLIANCE, not in 
> > OBJECT-TYPE,
> > b) in this case, all of the accessible objects in this table have a
> > MIN-ACCESS of read-only.  So, the answer to your question is already
> > present in the document.
> > 
> 
> Mmm... I thought the MIN-ACCESS in the module compliance is 
> to indicate
> that this MIB module can be implemented read-only. 
> It seems to me that if the object is supported in READ-WRITE 
> or READ-CREATE
> mode, that in that case the normal StorageType requirments 
> (as in RFC2579)
> apply. So I am not sure I agree with your argumentation here.
> Anyway, if you add the sentence, then the point is moot (at 
> least for this
> document).
> 
> > > - Mmm. I find the way you delete a row in t11FspfIfRowStatus
> > >   a bit strange/weird. But I guess I can live with it.
> > >   Can you explain why you do it this way?
> > 
> > Because:
> > 
> > 1. having unnecessary rows in this table clutters it up, and makes
> >    those rows that are useful harder for a NMS to find.
> > 2. having the current value always be in same object, 
> irrespective of
> >    whether it is a default or a non-default value is desirable. 
> > 3. when non-default values are specified for a port's parameters,
> >    they need to be retained in the MIB even when the port 
> is 'isolated'.
> > 4. when an E_Port becomes isolated, we want its row to get deleted
> >    if and only if all of its parameter values are the 
> default values.
> > 
> 
> Thanks. Might be good if you add some of that explnanation. 
> But I won't
> loose sleep if you (and the WG) decide that your current text 
> is good enough.
> 
> > > - t11FspfLinkIndex probably better specifies a range that 
> excludes zero ??
> > >   see also smicng error above.
> >  
> > The smicng error is confused, but I agree it would be better 
> > to exclude zero, and I'll change it to do so.
> > 
> 
> So I use the SMICng warning/error messages to check what the 
> situation is.
> And doing so made me wonder about the zero value for the INDEX.
> 
> > > admin/nits and just questions for clarity/explanation
> > > 
> > > - I see:
> > >     t11FspfIfNbrPortIndex OBJECT-TYPE
> > >         SYNTAX      Unsigned32 (1..4294967295)
> > > 
> > >     t11FspfLinkPortIndex OBJECT-TYPE
> > >         SYNTAX      Unsigned32 (0..16777215)
> > > 
> > >     t11FspfLinkNbrPortIndex OBJECT-TYPE
> > >         SYNTAX      Unsigned32 (0..16777215)
> > > 
> > >   Is it just me who wonders about the different ranges??
> > >   Maybe I am missing something or misunderstanding.
> >      
> > The format of the FSPF protocol packets as shown in FC-SW-4 
> indicates
> > that the port number field is 3 bytes long.  Hence, (0..16777215) is
> > correct, and (1..4294967295) is incorrect -- I will change
> > t11FspfIfNbrPortIndex.
> > 
> 
> Thanks
> 
> > > - I wonder what a value of zero means for T11FspfLinkType
> > >   If it can never exist, would it be better to make the range
> > >   (1..255) ??
> >  
> > The only difference between the definition in the MIB and 
> the definition
> > pointed to by the REFERENCE clause is that FC-SW-4 says "all others"
> > rather than just "others", i.e., zero is a reserved value.  
> I can add
> > "all" into the DESCRIPTION.
> > 
> 
> Would be good I think
> 
> > > - I wonder if T11FspfLsrType and T11FspfLinkType would be better
> > >   defined with a base type of INTEGER? In other words 
> Enumerations?
> >  
> > This is a trade-off between having the MIB definition being more
> > explicit or more open-ended.  As these objects are 
> currently defined,
> > they implicitly get any new values as and when T11 defines 
> new values
> > for that field.  Having them as enumerations would be more explicit,
> > but would require a MIB change to incorporate newly defined 
> values.  I
> > think this trade-off should be made individually for each 
> > object on its own merits.
> > 
> 
> So it seems thought has gone into this.
> If this is what the WG wants, then fine.
> 
> > > - I wonder how new non-Vendor-Specific values for T11FspfLsrType 
> > >   and T11FspfLinkType will be assigned/allocated?
> > >   Probably by T11? Would not hurt to say something about that.
> > 
> > Both of those TC's have a REFERENCE clause pointing to a T11 spec.
> > That particular spec doesn't define the term "Vendor Specific" (and 
> > I'm not sure which, if any, T11 spec does define it), but if I
> > understand correctly, in T11 the term means that it is the 
> vendors who
> > define such values and that different vendors can (and do) have
> > different meanings for the same value, i.e., they are reserved but
> > *not* allocated by an IANA-equivalent organization.  A 
> network manager
> > managing a Fibre Channel has to know this, not because of 
> SNMP or the
> > MIB, but because of Fibre Channel.
> > 
> 
> Might want to add some explanatory text then.
> 
> > > - I suspect that the RFCyyyy in t11FspfAdminStatus 
> REFERENCE clause
> > >   is (or should be) a different yyyy than the one in the
> > >   MODULE-IDENTITY macro?
> > 
> > Right. I'll change t11FspfAdminStatus to use zzzz.
> >
> AS you see, I can still be very nitpicky ;-)
>  
> > > - In a number of places I would prefer to see "MIB module" instead
> > >   of just "MIB". But I cam live with if if this is the 
> doc does not
> > >   need changing for anything else.
> > 
> > I'll look for them.
> >
> Thanks
>  
> > > - Section 7 is redundant and contains same material as the 
> > back matter.
> >  
> > Right.
> > 
> 
> Bert
> > Thanks,
> > Keith.
> > 
> > _______________________________________________
> > imss mailing list
> > [email-masked]
> > https://www1.ietf.org/mailman/listinfo/imss
> > 
> 
> _______________________________________________
> imss mailing list
> [email-masked]
> https://www1.ietf.org/mailman/listinfo/imss
> 

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