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

From: Keith McCloghrie <[email-masked]>
Date: Tue, 7 Mar 2006 07:43:15 -0800 (PST)

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

In starting to do the edits, I have found one other thing which it
might be appropriate to change at this time.

The present MIB, draft-ietf-imss-fc-fspf-mib-01.txt, defines the
object:

  t11FspfARegionNum OBJECT-TYPE
      SYNTAX      T11FspfARegionNum
      MAX-ACCESS  read-write
      STATUS      current
      DESCRIPTION
             "The AR number of this switch in this Fabric."
      REFERENCE  "Fibre Channel - Switch Fabric - 4 (FC-SW-4), Rev 7.5,
                 May 2005, section D.3.1."
      DEFVAL     {0}
      ::= { t11FspfEntry 2 }

I was starting to update the REFERENCE clauses to refer to the latest
rev of FC-SW-4, but in doing so I find that Annex/section D.3.1 is
missing.  In fact, the whole of what was Annex D has been removed from
the latest revision (rev 7.7 of December 2005).  I knew that T11 was
deprecating the concept of an "FSPF-Backbone" which was defined as:

    The FSPF-Backbone Fabric consists of multiple Autonomous Regions
    (AR) that are interconnected by a backbone network.

What I didn't realise was that it would disappear from the FC-SW-4
specification so soon.

Without that Annex, the definition of t11FspfARegionNum no longer
makes any sense.

So, the question is:

- should the T11-FC-FSPF-MIB retain the definition of t11FspfARegionNum,
  and do so by referring to an out-of-date T11 specification, or,

- should the definition of t11FspfARegionNum be removed from the
  T11-FC-FSPF-MIB at this time ??

(Note that the edits that would be needed to remove it are obvious and
straight-forward, e.g., the T11FspfARegionNum TC which is only used by
t11FspfARegionNum would be deleted; t11FspfARegionNum would deleted
from both the MODULE-COMPLIANCE clause and from the t11FspfGeneralGroup
group.)

Keith.

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

---

From: Keith McCloghrie <[email-masked]>
Date: Tue, 7 Mar 2006 07:23:40 -0800 (PST)

Hi Bert,
 
> Summary:

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

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

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

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

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

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

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

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

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

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

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

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

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

> - Section 7 is redundant and contains same material as the back matter.
 
Right.

Thanks,
Keith.

---

From: Keith McCloghrie <[email-masked]>
Date: Wed, 8 Mar 2006 07:21:01 -0800 (PST)

Bert,

Thanks for the review.

> This document is ready for IETF Last Call.
> 
> One topic that I would prefer to see addressed:
> 
> - When I see:
>     t11FcRouteDestAddrId OBJECT-TYPE
>        SYNTAX      FcAddressIdOrZero
>        MAX-ACCESS  not-accessible
>        STATUS      current
>        DESCRIPTION
>            "The destination Fibre Channel Address Identifier of
>            this route.  A zero-length string for this field is
>            not allowed."
>        ::= { t11FcRouteEntry 1 }
> 
>   I then wonder why the syntax is not:
> 
>       SYNTAX      FcAddressIdOrZero (SIZE(3))
> 
>   So that the restriction that zero-length is not allowed is
>   also machine readable.
 
While I agree in principle, I think it also has one other effect: 
it changes t11FcRouteDestAddrId from being a variable-length string
into a fixed-length string, which only matters because 
t11FcRouteDestAddrId is present in the INDEX clause, and thus,
I fear that some implementations might get the INDEX-ing wrong (re:
difference between bullets 2 and 3 at top of RFC 2578's page 28).
Thus, if you really want to see the restriction reflected in the
SYNTAX clause, I would prefer to do so as:

       SYNTAX      OCTET STRING (SIZE (3))

so that it uses a regular construct, and implementors will immediately
know what to do.  Do you agree ?

>   Could be addressesd after IETF Last Call, even with an RFC-Editor note.
> 
> Mainly have some NITs below.
> You may consider them as initial IETF Last Call comments.
> Let me know if you rather do a new rev first or if you
> prefer to do IETF LC now. I will probably let IETF LC extend
> beyond the IETF week, because people are probably busy reviewing
> documents for the IETF week itself.
 
I'd prefer to fix them now, but I'll wait for your response to this
message before doing so.

> - t11FcRouteRowStatus has a pretty meager DESCRIPTION clause
>   For example, from the DESCRIPTION clause of t11FcRouteIfDown
>   it seems that maybe a 'destroy' to the RowStatus object
>   may not take immediate effect? You might want to describe that.
>   It is also unclear if any writeable objects can be written
>   when a row is active?
 
The last sentence of RowStatus's DESCRIPTION in RFC 2579 says that
a 'destroy' requires the row to be removed immediately.  There was
discussion in T11.5 of the relationship between this table and the
routing mechanisms that a FC switch uses, and we agreed that such a
relationship is proprietary, and that the MIB needs to stay at
arms-length from being too specific about this relationship.  Thus, I
would prefer not to add text talking about it.

There is one thing I can add, which is implicit in t11FcRouteTable's
DESCRIPTION, but it would probably be useful to add a more explicit
statement in t11FcRouteRowStatus's DESCRIPTION:

         The only rows which can be deleted by setting this object to
         'destroy' are those for which t11FcRouteProto has the value
         'netmgmt'.

Then, it won't be so meagre :-).
 
> - The 4 OBJECT clauses that you did as comments in the
>   MODULE-COMPLIANCE are normally put as comments inside the
>   DESCRIPTION clause of the MODULE-COMPLIANCE clause itself.
>   That way the text is better kept when MIB module gets
>   extracted from RFC. Not a blocking comment though.
 
OK, I'll move them.  (The downside is that the double-quotes have to
change, e.g., to single-quotes).

> - Section 7 seems redundant with the back matter, and might as
>   well be removed.
 
I'll let the RFC Editor do that.

> - In the references section, are the details for [FC-SW-4] now
>   known? If so, might want to fill them out.

Yes, we filled them in recently on one of the other FC MIBs.
Thanks for reminding me of that.

Keith.

---

From: Keith McCloghrie <[email-masked]>
Date: Mon, 13 Mar 2006 08:25:39 -0800 (PST)

Hi,

I didn't see any response to my message below.  So, I propose to
pick one of the solutions below and ask for objections.  Specifically,
I will remove the definition of t11FspfARegionNum from the 
T11-FC-FSPF-MIB unless I hear objections.  I plan to submit the
updated I-D at the beginning of next week when (I think) I-D submission
re-opens.

Thanks,
Keith.
--------------
Forwarded message:
> From: Keith McCloghrie <kzm>
> Message-Id: <[email-masked]>
> Subject: Re: AD review of: draft-ietf-imss-fc-fspf-mib-01.txt
> To: [email-masked] (Keith McCloghrie)
> Date: Tue, 7 Mar 2006 07:43:15 -0800 (PST)
> Cc: [email-masked], [email-masked], [email-masked], [email-masked],
>         [email-masked], [email-masked], [email-masked], [email-masked]
>

---

From: Claudio DeSanti <[email-masked]>
Date: Mon, 13 Mar 2006 11:26:26 -0800

Keith,

your proposal makes sense to me.
Thanks,

                       Claudio.