Subject: [Ips] RE: Submitted iscsi-mib-10 draft

From: "Wijnen, Bert (Bert)" <[email-masked]>
Date: Fri, 9 Sep 2005 23:01:03 +0200

Sorry that it took me so long.

Summary:

I think it would be good to have a new revision that addresses
most (if not all) of my comments below.

I note that the draft-ietf-ips-auth-mib-06.txt is a normative
reference and that one is in status "Revised ID needed"
so I am not sure I am supposed to do MIB DOctor review
now or wait for a new revision?

Bert

----- details of the review

- I wonder if this part (from sect 6.16)
   To avoid sending an excessive number of notifications due to multiple
   errors counted, an SNMP agent implementing the iSCSI MIB module
   SHOULD NOT send more than three iSCSI notifications in any 10-second
   period.
  Should not be provided somehwer in one or more DESCRIPTION clauses
  inside the MIB itself. Maybe in the DESCRIPTION clauses of the
  Notifications them selves. This to ensure that people who implement
  the notifications WILL see this warning.

- smicng tells me:
     W: f(iscsi.mi2), (2620,9) Duplicate item "iscsiPortalStorageType"
     in object- group "iscsiPortalAttributesGroup" OBJECTS list
  The object is just listed twice. Simple to fix

- smilint tells me:
   ./ISCSI-MIB:92: [5] {integer-misuse} warning: use Integer32
     instead of INTEGER in SMIv2
  this is for IscsiTransportProtocols TC
   ./ISCSI-MIB:1064: [5] {integer-misuse} warning: use Integer32
     instead of INTEGER in SMIv2
  this is for iscsiNodeErrorRecoveryLevel
   ./ISCSI-MIB:2128: [5] {integer-misuse} warning: use Integer32
     instead of INTEGER in SMIv2
  this is for iscsiSsnErrorRecoveryLevel

  Probably better to use Integer32

- I wonder if IscsiTransportProtocols not better be IscsiTransportProtocol
  that is singular. Each time it is used for SYNTAX, I think it can
  only represent ONE SINGLE protocol from the list of valid protocols
  under iana registration.

- When I see this:
   IscsiName ::= TEXTUAL-CONVENTION
    DISPLAY-HINT  "223a"
    STATUS        current
    DESCRIPTION
        "This data type is used to define an iSCSI Name."
    REFERENCE
        "iSCSI Protocol Specification, Section 3.2.6, iSCSI Names."
    SYNTAX        OCTET STRING (SIZE(16..223))

  Then that REFERENCE looks nice, but I have no idea where to look?
  Is it RFC3720? If so why not do
    REFERENCE
        "RFC3720, iSCSI Protocol Specification, Section 3.2.6,
         iSCSI Names."
  If it is that section, then I do see it has a max size of 223
  specified in sect 3.2.6.1.
  I do not see the min size of 16 ?? I suspect it is actually larger
  than 16 (if I understand all subsections of 3.2.6 correct. It is
  not a showstopper, I just tried to understand.

  More important is that (if I understand it all):
    3.2.6.2.  iSCSI Name Encoding

    An iSCSI name MUST be a UTF-8 encoding of a string of Unicode
    characters with the following properties:

  So should the TC above not say something about it being UTF8?
  Maybe it should be a Utf8String as defined in SysApplMib (RFC2287)

  In any event, it would be important to indicate if the names
  in an OBJECT of this SYNTAX have gone through normalization
  (stringprep) or not. Or so I think.

  The  DISPLAY-HINT  "223a" seems to indicate that the OCTET STRING
  is a ASCII string, but I doubt it is based on my reading of rfc3720.
  If the string is UTF-8 (as I understand it to be) then the
  DISPLAY-HINT "223t" should be used as per bullet 3, page 22 of
  RFC2579.

  Further I see that when you actually use this TC to specify the
  SYNTAX of an object that one time you have DESCRIPTION
    iscsiInstLastSsnRmtNodeName
    DESCRIPTION
        "An octet string describing the name of the remote node

  and anotehr time a DESCRIPTION
    iscsiNodeName
    DESCRIPTION
        "A character string that is a globally unique identifier for
        this iSCSI node.  The node name is independent of the location

  and another time a DESCRIPTION
    iscsiTgtLastIntrFailureName
    DESCRIPTION
        "An octet string giving the name of the initiator
        that failed the last login attempt."

  which just makes it all more confusing. I.e. is it an octet or a
  character string. Is it UTF-8 or ASCII? Or what?

  The idea of a TC is to have one single DESCRIPTION of th esemantics
  of the object type (or so I think).

- Is there any discontinuity timer for: iscsiInstSsnFailures?
  I think it is this one: iscsiInstDiscontinuityTime
  But it needs to be specified in the DESCRIPTION clause of each
  Counter32/Counter64 as per RFC2578 sect 7.1.6

  Pls check for other Counter32/Counter5=64 objects as well.

  I would not be so explicit in the DESCRIPTION clause of 
  iscsiInstDiscontinuityTime itself. It makes it harder to use
  that same timer for future Counterxx objects.

- I see
    iscsiPortalAddr OBJECT-TYPE
    SYNTAX        InetAddress
    MAX-ACCESS    read-create
    STATUS        current
    DESCRIPTION
        "The portal's Internet Network Address."

  Formally the DESCRIPTION clause MUST indicate which InetAddressType
  object is used to define the format of this object.

  Also... is DNS possible? If so, one MUST specify when the dns name
  is to be resolved.

  Pls check this for all occurences of InetAddress

- I see
   iscsiNodeInitialR2T OBJECT-TYPE
    SYNTAX        TruthValue
    MAX-ACCESS    read-only
    STATUS        current
    DESCRIPTION
        "This object indicates the InitialR2T preference for this
        node:
        True = YES,
        False = will try to negotiate NO, will accept YES "

  nit: I would use all lowercase for 'true' and 'false' since that
  are the labels for TruthValue

  Similar remark for other cases where a TruthValue is used for 
  SYNTAX

- For the read-write objects in iscsiNodeAttributesTable I do not see
  any text that explains the required/expected persistence behaviour?

  Pls check that such behaviour is documented for all writable
  (read-create and read-write) objects.

- When I see
   iscsiNodeErrorRecoveryLevel OBJECT-TYPE
    SYNTAX        INTEGER (0..255)
    MAX-ACCESS    read-write
    STATUS        current
    DESCRIPTION
        "The ErrorRecoveryLevel preference of this node.
        Currently, only 0-2 are valid.
        This object is designed to accommodate future error recover
        levels.

  Then I wonder if you want to specify that 0-2 are only valid now
  in the MODULE-COMPLIANCE, or in other words, ensure that a
  implementation can claim compliance if they only support 0-2
  now.

- I see:
   iscsiSsnInitiatorName OBJECT-TYPE
    SYNTAX        IscsiName
    MAX-ACCESS    read-only
    STATUS        current
    DESCRIPTION
        "If iscsiSsnDirection is Inbound, this object is an
        octet string that will contain the name of the remote
        initiator.  If this session is a discovery session that
        does not specify a particular initiator, this object
        will contain a zero-length string.
        If iscsiSsnDirection is Outbound, this object will
        contain a zero-length string."

  But the SYNTAX clause of the IscsiName TEXTUAL CONVENTION says:

      SYNTAX        OCTET STRING (SIZE(16..223))
 
  SO a zero length string seems to be in conflict with that.

- iscsiSsnInitiatorAlias OBJECT-TYPE
    SYNTAX        SnmpAdminString
    MAX-ACCESS    read-only
    STATUS        current
    DESCRIPTION
        "An octet string that gives the alias communicated by the
        initiator end of the session during the login phase.

   Mmm... "An octet string" may confuse people. SnmpAdminString is
   a UTF-8 string. 

   Same for: iscsiSsnTargetAlias

- When I see:
    iscsiCxnLocalAddrType          InetAddressType,
    iscsiCxnLocalAddr              InetAddress,
    iscsiCxnProtocol               IscsiTransportProtocols,
    iscsiCxnLocalPort              InetPortNumber,
    iscsiCxnRemoteAddrType         InetAddressType,
    iscsiCxnRemoteAddr             InetAddress,
  Then I always wonder oif the iscsiCxnLocalAddr and iscsiCxnRemoteAddr
  MUST be of the same InetAddressType. If so, then you only need
  to have one such InetAddressType object and probably that is what 
  you want in that case, because it makes it easier to ensure that
  both address types are the same.

  See also my earlier remakrs about InetAddress (i.e. need to state
  which InetAddresType object specifies the format and need to say
  when DNS names are resolved (if they are valid types).

- iscsiCxnLocalPort OBJECT-TYPE
    SYNTAX        InetPortNumber
    MAX-ACCESS    read-only
    STATUS        current
    DESCRIPTION
        "The local transport protocol port used by this connection."
 
  According to RFC3291 (now obsoleted by RFC4001 by the way) you MUST
  describe what the meaning is of a portnuumber value zero.

  same for iscsiCxnRemotePort

- I do not understand why you do:

   iscsiNotifications OBJECT IDENTIFIER ::= { iscsiMibModule 0 }

   ...

   iscsiNotificationsPrefix OBJECT IDENTIFIER ::= { iscsiNotifications 0 }

  cause that gives you an OID for notifications that ends in 0.0.x
  and that I think we do not want. So I would just root the notifications
  under iscsiNotifications and forget about the xxxPrefix.

- I see comment lines like:

    ------------------------------------------------------------------
  
  They are OK but I find them dangerous, cause they MUST be a multiple
  of two dashes othersise it causes trouble. If you need/want a 
  separator line I prefer to see:

    --****************************************************************
  
- I see:
   iscsiComplianceV1 MODULE-COMPLIANCE
    STATUS current
    DESCRIPTION
        "Initial version of compliance statement based on
        initial version of this MIB module.

        If an implementation can be both a target and an
        initiator, all groups are mandatory."

   In principle there is nothing wrong. I personally always wonder if
   it does not make more sense to make that 2 Compliances, one for
   targets and one for initiators. It makes it much easier to 
   specify compliance in "mandatory groups" as opposed to large
   sets of optional or conditionally optional groups.

- You have the SCSIMIB as Informative reference.
  But you have RowPointer(s) pointing to entries in that MIB
  module, so to me that sort of indicates it is a normative
  reference, no?

Just for completeness:

$ idnits draft-ietf-ips-iscsi-mib-10.txt
idnits 1.77 (21 Aug 2005)

draft-ietf-ips-iscsi-mib-10.txt:

  Checking nits according to http://www.ietf.org/ID-Checklist.html:
    Checking conformance with RFC 3978/3979 boilerplate...
  * The document seems to lack an RFC 3978 Section 5.1 IPR Disclosure
    Acknowledgement -- however, there's a paragraph with a matching beginning.
    Boilerplate error?
    (The document uses RFC 3667 boilerplate or RFC 3978-like boilerplate
    instead of verbatim RFC 3978 boilerplate.  After 6 May 2005, submission of
    drafts without verbatim RFC 3978 boilerplate is not accepted.)

Bert

> -----Original Message-----
> From: Mark Bakke [mailto:[email-masked]]
> Sent: Sunday, February 20, 2005 20:35
> To: IPS
> Cc: Bert Wijnen; Keith McCloghrie
> Subject: Submitted iscsi-mib-10 draft
> 
> 
> 
> I've just submitted iscsi-mib-10 as an internet draft, to address MIB 
> Doctor review
> comments as well as clean up a few things.  Until it pops out of the 
> draft queue, it's
> available in:
> 
> ftp://ftpeng.cisco.com/mbakke/ips/iscsi-mib/
> 
> A new UML model showing the one update to the MIB structure
> (the addition of a node index to the initiator and target portal
> structure to address comments from long ago) is also available in
> the same location, along with the plain .mi2 for compiling, and
> diffs of the MIB module itself from -09.
> 
> Thanks to Jim Muchow, Keith McCloghrie, and Marjorie Krueger
> for help in working through this last set of edits, and to Bert
> Wijnen (MIB Doctor) for his constructive (and educational, for
> me anyway) comments.
> 
> Major changes to this module are:
> 
> - The addition of node index to initiator and target portals
> - The addition of StorageType objects
> - Better text for all RowStatus objects
> - Better text for all Index objects
> - Addition of DiscontinuityTime objects for instances and nodes
> - Improved SNMP terminology throughout
> - Some renumbering of higher-level objects was necessary to
>    make use of more standard numbering schemes and to add
>    the index values on the portals
> - A large number of REFERENCE statements were added
> - Changed objects from INTEGER to Unsigned32 except for enums
> - Renamed iscsiSsnDigestErrors to iscsiSsnCxnDigestErrors
> 
> As there were quite a few changes to this document, it is very likely
> that I've either missed something in the editing, or perhaps have not
> completely addressed the review comments.  Please let me know
> if there's anything missing.
> 
> Thanks,
> 
> Mark A. Bakke
> Cisco Systems
> [email-masked]
> 952-967-8374
> 
> 
> 

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

---

From: [email-masked]
Date: Mon, 19 Sep 2005 11:39:34 -0400

Bert,

I believe that Mark now owes you new revisions of both drafts
(iSCSI MIB and iSCSI Auth MIB).

Mark should consider this a public nudge to produce them ... ;-)

Thanks,
--David
----------------------------------------------------
David L. Black, Senior Technologist
EMC Corporation, 176 South St., Hopkinton, MA  01748
+1 (508) 293-7953             FAX: +1 (508) 293-7786
[email-masked]        Mobile: +1 (978) 394-7754
----------------------------------------------------

> From: [email-masked] [mailto:[email-masked]] On 
> Behalf Of Wijnen, Bert (Bert)
> Sent: Friday, September 09, 2005 5:01 PM
> To: Mark Bakke; IPS
> Cc: Keith McCloghrie; Allison Mankin (E-mail)
> Subject: [Ips] RE: Submitted iscsi-mib-10 draft
>