RE: draft-ietf-sip-mib-11.txt [ was: [MIB-DOCTORS] FW: Agenda and Package for August 3, 2006 Telechat PRELIMINARY]

["Kevin Lingle \(klingle\)" <[e-mail]>]

bert,

thank you for your comments.   jean-francois and i will look
at incorporating what we can.    i think we both have pretty
limited bandwidth to devote to sip-mib these days.  it has been
through several last calls w/in sip wg.   we do want to produce
a quality mib and your expertise/comments are important considering
your experience with std mibs.  so again thanks.  we'll prepare
a response soon (i hope ;)

kevin 

-----Original Message-----
From: Wijnen, Bert (Bert) [mailto:[e-mail]] 
Sent: Monday, July 31, 2006 4:10 PM
To: Romascanu, Dan (Dan); Kevin Lingle (klingle); Jean-Francois Mule
(E-mail); [e-mail]; [e-mail]
Cc: [e-mail]; Cullen Jennings (fluffy); Dean Willis (E-mail)
Subject: draft-ietf-sip-mib-11.txt [ was: [MIB-DOCTORS] FW: Agenda and
Package for August 3, 2006 Telechat PRELIMINARY]

Dan, w.r.t
> 2.1 WG Submissions
> 2.1.1 New Item
>   o draft-ietf-sip-mib-11.txt
>     Management Information Base for the Session Initiation Protocol
(SIP) 
>     (Proposed Standard) - 1 of 3 
>     Note: PROTO Shepherd is Dean Willis 
>     Token: Cullen Jennings

In general, I do have some concerns/issues that I think BEST be fixed.
I have numerous editorial suggestions. Once could claim that such can be
ignored at this stage in the process, and I would have to agrre.
At the other hand, specifically the naming consistency bothers/worries
me a lot, even though it is not a fatal flaw. I understand that it will
be quite an effort to adopt to a consistent naming scheme, yet I
personally think it would be very worthwhile. Not only for helping to
avoid future name clashes, but also to make the descriptors more
self-explanatory as to which MIB module and/or which table they belong
to.

Sorry for not having done a review earlier (like at IETF last call or
so).

Bert
--- SIP-TC module

None of this is blocking or a fatal flaw. But pls DO consider my
well-intended comments to try and help make this MIB module better and
easier to understand by MIB implementers and NM application developers.

When I see:
      SipTransportProtocol ::= TEXTUAL-CONVENTION
              STATUS current
              DESCRIPTION
                   "This convention is a bit map.  Each bit represents a
                    transport protocol.  If a bit has value 1, then that
                    selected transport protocol is in some way dependent
                    on the context of the object using this convention.
                    If a bit has value 0, then that transport protocol
                    is not selected.  Combinations of bits can be
                    set when multiple transport protocols are selected.

                    bit 0 : a protocol other than those defined here
                    bit 1 : User Datagram Protocol
                    bit 2 : Transmission Control Protocol
                    bit 3 : Stream Control Transmission Protocol
                    bit 4 : Transport Layer Security Protocol over TCP
                    bit 5 : Transport Layer Security Protocol over SCTP"
              SYNTAX     BITS {
                               other(0),  -- none of the following
                               udp(1),

                               tcp(2),
                               sctp(3),   -- RFC4168
                               tlsTcp(4),
                               tlsSctp(5) -- RFC 4168
              }
   --         REFERENCE "RFC 3261, Section 18"
   --         REFERENCE "RFC 4168"

Then I wonder why the REFERENCE is done with ASN.1 comments instead of
using a real REFERENCE clause. The following would achive that:
      SipTransportProtocol ::= TEXTUAL-CONVENTION
              STATUS current
              DESCRIPTION
                   "This convention is a bit map.  Each bit represents a
                    transport protocol.  If a bit has value 1, then that
                    selected transport protocol is in some way dependent
                    on the context of the object using this convention.
                    If a bit has value 0, then that transport protocol
                    is not selected.  Combinations of bits can be
                    set when multiple transport protocols are selected.

                    bit 0 : a protocol other than those defined here
                    bit 1 : User Datagram Protocol
                    bit 2 : Transmission Control Protocol
                    bit 3 : Stream Control Transmission Protocol
                    bit 4 : Transport Layer Security Protocol over TCP
                    bit 5 : Transport Layer Security Protocol over SCTP"
              REFERENCE "RFC 3261, Section 18 and RFC 4168""
              SYNTAX     BITS {
                               other(0),  -- none of the following
                               udp(1),

                               tcp(2),
                               sctp(3),   -- RFC4168
                               tlsTcp(4),
                               tlsSctp(5) -- RFC 4168
              }

Similar comment for: SipOptionTagHeaders

Would be good to have a REFERENCE for the SipEntityRole and
SipMethodName as well. 

It is interesting to see that the DESCRIPTION clause of the
MODULE-IDENTITY in the SIP-COMMON-MIB has all the details about the
different roles of SIP entities, while the SIP-TC MIB module, not the
specific SipEntityRole TC do NOT have such a nice/good description. I
think that all that text would be much better provided in the
SipEntityRole TC.

Specifically for SipMethodName I seem unable to find what values exactly
this SYNTAX (i.e. ASCII? UTF-8 ?? any octet
value?) can take and why its range is (1-128).
The 128 max length causes a warning from SMI SYNTAX checkers because it
can cause OIDs with ore than 128 subIDs when used as an index (which is
done in for example the SIP-COMMON-MIB).
I see the RFC-Editor notes suggesting to change it to max 119 octets,
but I see no base as to why that would be a reasonable limit. In the
SIP-COMMON-MIB I see sipMethodName for the ??same??
methodName, and the use of SnmpAdminString for that occurrence seems to
indicate it IS UTF-8 and max 32 octets !!??


----- SIP-COMMON-MIB

First some comments that are flawed (item 3 below, the badValue error
code is (in my view) even fatally flawed in that it breaks the SNMPv2
protocol operations of RFC3416).In any event, I think these should be
SERIOUSLY considerede for a fix/change

1. SYNTAX checking:

  C:\bwijnen\smicng\work>smicstrict
  C:\bwijnen\smicng\work>smicng SIP-COMMON-MIB.inc
  W: f(rfc2788.mi2), (562,3) OBJECT-GROUP "applGroup" is not 
     used in a MODULE-COMPLIANCE in current module
  W: f(SIP-COMMON-MIB), (1026,7) Row "sipMethodStatsEntry" has 
     indexing that may create variables with more than 128 sub-ids
  W: f(SIP-COMMON-MIB), (1098,7) Row "sipStatusCodesEntry" has
     indexing that may create variables with more than 128 sub-ids
  W: f(SIP-COMMON-MIB), (1206,7) Row "sipStatusCodeNotifEntry" has
     indexing that may create variables with more than 128 sub-ids
  W: f(SIP-COMMON-MIB), (1405,7) Row "sipCommonStatsRetryEntry" has
     indexing that may create variables with more than 128 sub-ids

  *** 0 errors and 5 warnings in parsing

  The first warning is from another MIB-module, so we can ignore it.

  W.r.t. the last 4 warnings, I think it would be good (as recommended
  by RFC4381 sect 4.6.6.) to add some words in teh DESCRIPTION clauses
  about OID length limitations.

2.sipServiceNotifEnable is a writable object, but I see not details
  about the expected persistenmce behaviour! I strongly recommend to
  add that.

3.sipServiceNotifEnable DESCRIPTION clause suggests to return a badValue
  error. a badValue is NOT a value to be returned by SNMPv2c/v3. It is
  only there for backward SNMPv1 compatibility. So suggesting that as
  an error value in an SMIv2 MIB module is not correct!

  Not sure I completely understand DESCRIPTION. It seems like a SIP
entity can
  choose to support notifications or not. In the former case it must
support
  all of them, in the latter case it would support none at all. So it
seems
  one would not see an entity that supports only 2 uut of 3 or some
other
  mix?

  If my interpretation/understanding is correct (in any event, it would
be
  good to make it clear in the DESCRIPTION clause) I think the correct
  approach would be that the object is allowed to be supported in
read-only
  mode in case the entity does NOT support any notifications. This is
done
  via a MIN-ACCESS in the MODULE-COMPLIANCE.
  Another approach (which also allows a mix of supported/non-supported
  noticifcations, and so is the prefered way in my view) would be to
  return a error of "inconsistentValue".
  It cannot return badValue (see Elements of Procedure pages 19/20 of
  RFC3416). It could return inconsistentValue (step 10 on page 19/20
  of RFC3416) to achieve what you seem to want.

  So the simplest fix is to: s/badValue/inconsistentValue/
  and to clarify the DESCRIPTION clause as to what is exactly intended
  (no support for ALL notifications, or an option to support only 
   a subset)

4.I see no mention of which discontinuity timer applies to the counters
  in sipSummaryStatsTable. Same for sipStatusCodesTable,
  sipCommonStatsRetryTable and sipOtherStatsTable.

5.I see no persistency behaviour defined for created entries in the
  sipStatusCodesTable


None of the following is blocking and none of it causes a fatal flaw.
I think they would be good editorial changes though:

- I find the use of MAY in the DESCRIPTION clause of the MODULE-IDENTITY
  not correct. I would just use lowercase "may". It is not specifying
  any compliance requirements here (or so I think/perceive).

- I personally would rename sipCommonMIBNotifs into
sipCommonMIBNotifications
  Most other MIB modules use the longer "Notifications" instead of
abbreviations
  so I think it helps in overall MIB-wide naming consistency.

- Simliarly I would rename sipCommonMIBConform into
sipCommonMIBConformance
  as is done in most otehr MIB modules.

- it seems to me that sipServiceNotifEnable could have a DEFVAL clause
as
  follows:
    DEFVAL { { sipServiceColdStart, sipServiceWarmStart } }
  At least, that is also what the DESCRIPTION clause tells me. Would
  be good to specify that in machine redable form, i.e. using a DEFVAL
  clause.

- for sipPort, the DESCRIPTION clause should indicate what a value of
zero
  means. Probably a value of zero makes no sense here, and so the
  SYNTAX would probably best be specified as
         SYNTAX    InetPortNumber (1..65535)

- In terms of naming consistency, I would rename
      SipCommonCfgEntry ::=
          SEQUENCE {
                   sipProtocolVersion        SnmpAdminString,
                   sipServiceOperStatus      INTEGER,
                   sipServiceStartTime       TimeTicks,
                   sipServiceLastChange      TimeTicks,
                   sipOrganization           SnmpAdminString,
                   sipMaxTransactions        Unsigned32,
                   sipServiceNotifEnable     BITS,
                   sipEntityType             SipEntityRole
          }
  into:
      SipCommonCfgEntry ::=
          SEQUENCE {
                   sipCommonCfgProtocolVersion        SnmpAdminString,
                   sipCommonCfgServiceOperStatus      INTEGER,
                   sipCommonCfgServiceStartTime       TimeTicks,
                   sipCommonCfgServiceLastChange      TimeTicks,
                   sipCommonCfgOrganization           SnmpAdminString,
                   sipCommonCfgMaxTransactions        Unsigned32,
                   sipCommonCfgServiceNotifEnable     BITS,
                   sipCommonCfgEntityType             SipEntityRole
          }
   and I would also rename:
      SipPortEntry ::=
          SEQUENCE {
                   sipPort                 InetPortNumber,
                   sipTransportRcv         SipTransportProtocol
          }
   into something like:
      SipPortEntry ::=
          SEQUENCE {
                   sipPort                 InetPortNumber,
                   sipPortTransportRcv     SipTransportProtocol
          }
   maybe that while table should even have sipCommon as a prefix.

   That is, I would have all SIP-COMMON-MIB descriptors start with
sipCommon
   It is easy to ensure that there are no naming conflicts today, but if
   changes/additions/extensions are made in the future, it becomes all 
   so much easier if one has started with a very clear naming scheme;
   specifically with using a consistent prefix for descriptor-names
   within the same MIB module. 

   So this also applies to several other tables. For example,
   I would rename sipOptionTagTable to sipCommonOptionTagTable and
   have all objects there also start with sipCommonOption prefix.

- NIT: sipOptionTagEntry
  in description clause: s/at reboot/over a reboot or restart/

- consider sipOptionTagEntry
    SHOULD be non-volatile
  So an NM application has to do a trial and error to figure ity out?
  Why could this not be made a: MUST be non-volatile
  But then checking further, I see that the whole table is read-only,
  so no need to talk about persistency of the entries.

- sipOptionTag
  I would recommend to add a REFERENCE clause to point to sect 27.1
  of RFC3261. That helps understand what the actual values can be.
  Because acording to the SYNTAX, it could contain chinese, but
according
  to sect 27.1 of RFC3261 it cannot.

  I kind of like that you use SnmpAdminString (i.e. UTF-8 based), so
that
  you recognize that at some time there might be non US ASCII tags?
  But the SYNTAX does not represent the actual allowed content of the
  TAG. If that content is fixed to ALPHA/DIGIT from US ASCII set, then
  maybe it would be better to define your own TC as ipposed to using
  a TC that is intened for internationalized human readable strings.
 
  It might also be helfull to add some text that explains what to do
  if the length of the sipOptionTag exceed the max length of
  SnmpAdminString which seems allowed (MAY text in 27.1 of RFC3216).

- sipMethodSupportedTable
  Seems to have only read-only objects/columns, so I do not see
  a need or the usefullness to talk about persistyency (non-volatile)
  in the DESCRIPTION claise of sipMethodSupportedEntry

- sipMethodSupportedTable
  Maybe it is just me. But I am confused by the use of MAY (2 times).
  It gives me the impression that you do not HAVE TO include anything.
  But I would think that one MUST include ALL METHODS that the
  represented SIP instance supports, no?

- sipMethodName
  I think I have already spoken about this better be named
  sipCommonMethodSupportedMethodName. 
  But the SYNTAX is SnmpAdminString and I wonder why it would not
  be SipMethodName as from your own SIP-TC mib module.
  
- I would rename
      SipCommonCfgTimerEntry ::=
          SEQUENCE {
                   sipCfgTimerA               Unsigned32,
                   sipCfgTimerB               Unsigned32,
                   sipCfgTimerC               Unsigned32,
                   sipCfgTimerD               Unsigned32,
                   sipCfgTimerE               Unsigned32,
                   sipCfgTimerF               Unsigned32,
                   sipCfgTimerG               Unsigned32,
                   sipCfgTimerH               Unsigned32,
                   sipCfgTimerI               Unsigned32,
                   sipCfgTimerJ               Unsigned32,
                   sipCfgTimerK               Unsigned32,
                   sipCfgTimerT1              Unsigned32,
                   sipCfgTimerT2              Unsigned32,
                   sipCfgTimerT4              Unsigned32
          }
   into (for naming consistency):
      SipCommonCfgTimerEntry ::=
          SEQUENCE {
                   sipCommonCfgTimerA               Unsigned32,
                   sipCommonCfgTimerB               Unsigned32,
                   sipCommonCfgTimerC               Unsigned32,
                   sipCommonCfgTimerD               Unsigned32,
                   sipCommonCfgTimerE               Unsigned32,
                   sipCommonCfgTimerF               Unsigned32,
                   sipCommonCfgTimerG               Unsigned32,
                   sipCommonCfgTimerH               Unsigned32,
                   sipCommonCfgTimerI               Unsigned32,
                   sipCommonCfgTimerJ               Unsigned32,
                   sipCommonCfgTimerK               Unsigned32,
                   sipCommonCfgTimerT1              Unsigned32,
                   sipCommonCfgTimerT2              Unsigned32,
                   sipCommonCfgTimerT4              Unsigned32
          }

  Further, I cannot say that I find the naming of Timers with A, B, C,D
etc
  verty intuitive or user friendly.

- In the sipCommonCfgTimerTable I find it strange to see DEFVALs for 
  read-only objects.   Specifically strange if I see for example:
      sipCfgTimerD OBJECT-TYPE
          SYNTAX      Unsigned32 (0..300000)
          UNITS       "milliseconds"
          MAX-ACCESS  read-only
          STATUS      current
          DESCRIPTION
               "This object reflects the amount of time that the server
                transaction can remain in the 'Completed' state when
                unreliable transports are used. The default value MUST
                be greater than 32000 for UDP transport and its value

                MUST be 0 for TCP/SCTP transport."
          REFERENCE
                "RFC 3261, Section 17.1.1.2"
              DEFVAL { 32000 }
   where the DESCRIPTION clause suggests that the value might in fact
   be EITHER >32000 or 0. So how can there be a DEFVAL that covers both?

- I see:
      sipStatusCodeMethod OBJECT-TYPE
          SYNTAX      SipMethodName
          MAX-ACCESS  not-accessible
          STATUS      current
          DESCRIPTION
               "This object uniquely identifies a conceptual row
                in the table and reflects an assigned number used
                to identifier a specific SIP method."
   s/to identifier/to identify/ !!??

-    sipStatusCodeRowStatus OBJECT-TYPE
         SYNTAX      RowStatus
         MAX-ACCESS  read-create
         STATUS      current
         DESCRIPTION
              "The row augmentation in sipStatusCodeNotifTable
               will be governed by the value of this RowStatus.

               This object is REQUIRED to create or delete rows
               by a manager.

               The values 'createAndGo' and 'destroy' are the
               only valid values allowed for this object.
               If a row exists, it will reflect a status of
               'active' when queried."
         ::= { sipStatusCodesEntry 5 }

   You are missing a statement that explains if writable columns 
   (they would be columns in the augmenting sipStatusCodeNotifTable)
   can be changed if the value of this object is 'active'. I would 
   think the answer is YES< they can be modified, and RFC2579 
   in RowStatus TC DESCRIPTION clause mandates that you specify
   this in the DESCRIPTION clause of an object that uses the
   RowStatus TC as its SYNTAX.

   The 2nd para in the DESCRIPTION clause seems redundant. But is
   acceptable. I would remove it though.

   The last para seems material that needs to go into the
   MODULE-COMPLIANCE. you are basically stating that a createAndWait
   and notInService are not required. I find it strange that you
   would want to PROHIBIT support for those. If the latter, than
   you would normally do that by a SYNTAX of:
      sipStatusCodeRowStatus OBJECT-TYPE
          SYNTAX      RowStatus {
                        active(1),
                        createAndGo(4),
                        destroy(6)
                      }
   If the former (I think I would prefer the former), in which case
   you leave the SYNTAX as is, but in the MODULE-COMPLIANCE, you add
   an OBJECT clause, as follows. 
   I would replace (i.e. add):
      sipCommonCompliance MODULE-COMPLIANCE

          STATUS     current
          DESCRIPTION
               "The compliance statement for SIP entities."

          MODULE -- this module
               MANDATORY-GROUPS { sipCommonConfigGroup,
                                  sipCommonStatsGroup }
   with:
      sipCommonCompliance MODULE-COMPLIANCE

          STATUS     current
          DESCRIPTION
               "The compliance statement for SIP entities."

          MODULE -- this module
               MANDATORY-GROUPS { sipCommonConfigGroup,
                                  sipCommonStatsGroup }

          OBJECT sipStatusCodeRowStatus
          SYNTAX RowStatus { active(1) }
          WRITE-SYNTAX RowStatus { createAndGo(4), destroy(6) }
          DESCRIPTION
            "Support for createAndWait and notInService is not
required."

- I would think that the DEFVAL for sipStatusCodeNotifEmitMode would
  better be oneShot so as to minimize network load. In any event, it
seems
  wierd to me that the value would be "triggered" by default (I see that
  the DESCRIPTION clause says that this is also possibly the default
value). 


- The way the MODULE-COMPLIANCE is defined means that IF you claim
  compliance, then for all writable objects you DO support them in
  read-write or read-create (as the case may be). As long as the
  WG and authors are aware of that and if that is what they want,
  then fine. I am assuming so, but did want to point it out during
  my review.


------ SIP-UA-MIB

I see no blocking issues or fatal flwas. However, pls consider:

- I would rename sipUAMIBConform to sipUAMIBConformance for consistency
  with many otehr MIB modules.

- I see:
      sipUACfgServerEntry OBJECT-TYPE
          SYNTAX     SipUACfgServerEntry
          MAX-ACCESS not-accessible
          STATUS     current
          DESCRIPTION
               "A row of server configuration.

                Each row represents those objects for a particular SIP
                user agent present in this system.  applIndex is used to
                uniquely identify these instances of SIP user agents and
                correlate them through the common framework of the
                NETWORK-SERVICES-MIB (RFC 2788). The same value of
                applIndex used in the corresponding SIP-COMMON-MIB is
                used here.
                The objects in this table entry SHOULD be non-volatile
                and their value SHOULD be kept at reboot."
   
   But the whole table is read-onlyu, so the last 2 lines seem useless
   and so not needed to me.

- I see:
     sipUACfgServerAddr OBJECT-TYPE
         SYNTAX     InetAddress
         MAX-ACCESS read-only
         STATUS     current
         DESCRIPTION
              "This object reflects the address of a SIP server
               this user agent will use to proxy/redirect calls."
         REFERENCE "INET-ADDRESS-MIB (RFC 4001)"
         ::= { sipUACfgServerEntry 3 }
  
   RFC4001, requires (MUST language) that every object with syntax
InetAddress
   specifies WHICH object of SYNTAX InetAddressType controls the format
of the
   InetAddress. It is simple. Add this text:

         The type of this address is determined by the value of the
         sipUACfgServerAddrType
         object.  Note that implementations must limit themselves

- real NIT.
  from a user-friendlyness/intuitiveness point of view, I think I would
  rename
  -  sipUACfgServerAddrType to sipUACfgServerAddressType
  -  sipUACfgServerAddr     to sipUACfgServerAddress
  -  sipUACfgServerFunction to sipUACfgServerRole.


------- SIP-SERVER-MIB

maybe in need of a fi:
1.sipNumProxyRequireFailures does not specify which (if any)
  discontinuity timer applies.
  Same for: sipUserAuthenticationFailures
  Same for counters in sipRegStatsTable

Editorial concerns (includes naming concern)

- I would rename sipServerMIBConform to sipServerMIBConformance for
  naming consistency with so many other MIB modules

- sipServerCfgEntry talks about "SHOULD be non-volatile"
  but the whole table is read-only, so I see no use/need for that text.

- In the DESCRIPTION clause of sipServerHostAddr, I see:
                  sipServerHostAddrType formalizes the type of address
                  given by this object.  It is the user's
                  responsibility to maintain consistency between this
                  object and the type specified by
                  sipServerHostAddrType."

  It does explain how the format is controlled (by value of
  sipServerHostAddrType). But it is unclear who the "user" is who is
made
  responsible for consitency here. I think it is an implementation
  responsibility, since both objects are read-only.

- I would rename sipProxyCfg to sipServerProcyCfg (and all descriptors
  with same prefix too).
  I would rename sipProxyStats to sipServerProxyStats (and all
  descriptors with same prefix as well).

- sipProxyCfgEntry talks about "SHOULD be non-volatile" while the whole
  table is read-only. So that text is meaningless/not-needed.

- I personally would rename sipProxyAuthRealm to
sipProxyAuthDefaultRealm
  to make the descriptor better cover the content.


Bert

_______________________________________________
MIB-DOCTORS mailing list
[e-mail]
https://www1.ietf.org/mailman/listinfo/mib-doctors