Subject: [MIB-DOCTORS] RE: MIB Doctor review of: draft-ietf-pwe3-tdm-mib-08.txt

From: "Orly Nicklass" <[email-masked]>
Date: Tue, 18 Dec 2007 16:55:13 +0200

Finally got the time to handle the comments.

Tnx for the detailed review.

New draft will be issued shortly

Orly Nicklass, Ph.D

Head of Research & Development

3 Hanagar St. Neve Ne'eman B

Hod Hasharon, Israel 

Tel: +972-9-7751290
Mobile: +972-54-2209565 

________________________________

From: WIJNEN, Bert (Bert) [mailto:[email-masked]] 
Sent: Wednesday, October 17, 2007 16:40
To: Orly Nicklass
Cc: [email-masked]; [email-masked]; MIB Doctors (E-mail)
Subject: MIB Doctor review of: draft-ietf-pwe3-tdm-mib-08.txt

First, I am not subscribed to the WG mailing list, so pls copy me if you
want me to see any comments/responses.

Pls note that revision 8 has expired. I have reviewed the copy I still
had on my machine.

Here are my comments.

C:\bwijnen\smicng\work>smicng pwTdm.inc
  
  E: f(pwTdm.mi2), (855,24) Index item "pwTDMPerfIntervalNumber" must
     be defined with syntax that includes a range
  E: f(pwTdm.mi2), (1027,23) Index item "pwTDMPerf1DayIntervalNumber"
     must be defined with syntax that includes a range

The above two SMICng errors can/should be fixed:

   pwTDMPerfIntervalNumber OBJECT-TYPE
     SYNTAX        Unsigned32
     MAX-ACCESS    not-accessible
     STATUS        current
     DESCRIPTION
         "A number (normally between 1 and 96 to cover a 24 hour
          period) which identifies the interval for which the set
          of statistics is available. The interval identified by 1
          is the most recently completed 15 minute interval, and
          the interval identified by N is the interval immediately
          preceding the one identified by N-1. The minimum range of
          N is 1 through 4. The default range is 1 through 32. The
          maximum value of N is 1 through 96."
     ::= { pwTDMPerfIntervalEntry 1 }

The DESCRIPTION clause already states that a SYNTAX of
     SYNTAX        Unsigned32 (1..96)
makes sense
[[Orly Nicklass]] done
And for

   pwTDMPerf1DayIntervalNumber OBJECT-TYPE
     SYNTAX        Unsigned32
     MAX-ACCESS    not-accessible
     STATUS        current
     DESCRIPTION
         "The number of interval, where 1 indicates current day
          measured period and 2 and above indicate previous days
          respectively"
     ::= { pwTDMPerf1DayIntervalEntry 1 }
[[Orly Nicklass]] done

The DESCRIPTION clause shows that zerop seem not to be a valid value.
There is probably also a max number of days that makes sense So a syntax
of Unsigned32 (1..30) or some such seems to make sense and be inline
with the range of pwTDMValidDayIntervals.

By the way, see also my comments on the pwATM MIB module. I personally
think that Unsigned32 is OK, but text in RFC2493 might even want us to
use INTEGER instead of Unsigned32. Maybe some old-time telecom experts
can tell us.

[[Orly Nicklass]] left as is

I see
     ::= { transmission XXX }
   -- RFC Editor: replace XXX with IANA-assigned number & remove this
   -- note. Please see IANA considerations section

And again I wonder if allocation under transmission is
correct/justified.
You do not want a PW to be an entry in the ifTable. And transmission is
for media-specific MIB data for an interface of a specific ifType.
SO I think allocation under mib-2 makes more sense.

[[Orly Nicklass]] done

I see:
   -- Tables, Scalars
   pwTDMObjects       OBJECT IDENTIFIER
                                ::= { pwTDMMIB 1 }
   -- Notifications
   pwTDMNotifications OBJECT IDENTIFIER
                                ::= { pwTDMMIB 2 }

   -- Conformance
   pwTDMConformance   OBJECT IDENTIFIER
                                ::= { pwTDMMIB 3 }

Why not follow RFC4181 suggestion (so all MIB modules are more
consistent)
and use instead:
   -- Notifications
   pwTDMNotifications OBJECT IDENTIFIER
                                ::= { pwTDMMIB 0 }

   -- Tables, Scalars
   pwTDMObjects       OBJECT IDENTIFIER
                                ::= { pwTDMMIB 1 }
   -- Conformance
   pwTDMConformance   OBJECT IDENTIFIER
                                ::= { pwTDMMIB 2 }
[[Orly Nicklass]] done
- In pwTDMEntry DESCRIPTION clause I see:
          Unless otherwise specified, all RW objects in this table
          MUST NOT be changed after row activation (see [PWMIB])
          and should remain unchanged after reboot."

  I guess that RW means read-write (or better writable)
  I guess with "should remain unchanged after reboot" you mean
  that the values must persis over a restart/reboot? I would
  make that wording a bit clearer then.
  I am not sure what "row activation" means here.
  The row gets created by the agent according to your description
  clause. So to me it seems that the row then is "active", no?[[Orly
Nicklass]] no
  Maybe you mean that the row becomes active once/if the base
  row in the generic pwTable becomes active?[[Orly Nicklass]] yes
  Anyway, you want to be cleaerer as to what you mean here.
  It is better to not use citations (like [PWMIB]) in DESCRIPTION
  clauses.
[[Orly Nicklass]] done
- For
   pwTDMCfgIndex   OBJECT-TYPE
     SYNTAX        PwTDMCfgIndex
     MAX-ACCESS    not-accessible
     STATUS        current
     DESCRIPTION
         "Index to an entry in this table. The value is a copy of the
          assigned pwTDMCfgIndexNext"
     ::= { pwTDMCfgEntry 1 }

  I am not sure I would use such a DESCRIPTION clause. Maybe better
  would be somethink aka:
     DESCRIPTION
         "Index to an entry in this table. When an NMS wants to create
          a new entry/row in this table, it best makes use of the value
          of the pwTDMCfgIndexNext object in order to find a free or
          available index value."[[Orly Nicklass]] done

- I see
   pwTDMCfgRowStatus    OBJECT-TYPE
     SYNTAX               RowStatus
     MAX-ACCESS           read-create
     STATUS               current
     DESCRIPTION
         "Object used for creating, modifying, and deleting
          a row from this table. The following objects should not be
          modified if the entry is in used and the status is active:
          pwTDMCfgPayloadSize, pwTDMCfgRtpHdrUsed,
          pwTDMCfgJtrBfrDepth, and pwTDMCfgPayloadSuppression.
          The row should not be deleted if the entry is in used"

  The wording about "should not be modified" indeed belongs here
  (and so I think some text in pwTDMCfgEntry DESCRIPTiON can
   be removed, namely:
          Unless otherwise specified, all objects in this table
          MUST NOT be changed after row activation (see [PWMIB])
          if the row index is in used by an entry in pwTDMTable.[[Orly
Nicklass]] left in
  I would also reword the text here a bit and instead of
  "should not be modfied" I would say "cannot be modified" or
  "MUST NOT be modified". That shows to agent implementers that
  they can reject such a change request, where-as a "should not"
  seems to be just friendly advise to an NMS.
  Just my subjective reading I guess.

  The last sentence again, I would say the row cannot be deleted.
  or the row MUST NOT be deleted.
[[Orly Nicklass]] done
  typo: "is in used" shodul be "is in use." I think
[[Orly Nicklass]] done
- For
   pwTDMCfgPktReorder OBJECT-TYPE
     SYNTAX        TruthValue
     MAX-ACCESS    read-create
     STATUS        current

     DESCRIPTION
         "If set True: as CE bound packets are queued in the
          jitter buffer, out of order packets are re-ordered. The
          maximum sequence number differential (i.e., the range in
          which re-sequencing can occur) is dependant on the depth
          of the jitter buffer. See pwTDMCfgJtrBfrDepth.

          NOTE: Some implementations may not support this feature.
          The agent is then required to set this to False."
     ::= { pwTDMCfgEntry 5 }

  I think I would change the last sentence to
          The agent should then reject a SET request for true.[[Orly
Nicklass]] done
  But it seems like this is an optional object, or as an object
  that could be read-only with the sole value of 'false'. This
  would be better expressed in the MODULE-COMPLIANCE I think.[[Orly
Nicklass]] done

- This one
   pwTDMCfgPayloadSuppression  OBJECT-TYPE
     SYNTAX        INTEGER
                    {
                       enable  ( 1),
                       disable ( 2)
                    }
  Could also be represented with a SYNTAX of TruthValue, for example
     pwTDMCfgEnablePayloadSuppression  OBJECT-TYPE
        SYNTAX        TruthValue
  That would mean re-use of an already existing TC.
  Not a major issue, just better re-use.[[Orly Nicklass]] stays as is
due to existing implementations

- W.r.t.
   pwTDMCfgConsecPktsInSynch          OBJECT-TYPE
     SYNTAX        Unsigned32
     MAX-ACCESS    read-create
     STATUS        current
     DESCRIPTION
         "The number of consecutive packets with sequential
          sequence numbers that are required to exit the
          LOPS state.
          Object  MAY be changed when the related PW is
          defined as not active."
     REFERENCE
         "SATOP"
     DEFVAL { 2 }
     ::= { pwTDMCfgEntry 9 }

   pwTDMCfgConsecMissPktsOutSynch  OBJECT-TYPE
     SYNTAX        Unsigned32
     MAX-ACCESS    read-create
     STATUS        current
     DESCRIPTION
         "The number of consecutive missing packets that are
          required to enter the LOPS state.
          Object  MAY be changed when the related PW is
          defined as not active."
     REFERENCE

  I am not sure if the value zero makes sense (ever)?[[Orly Nicklass]]
no
  If not, then I would exclude it. If it does, then may
  be some explanatory text is needed.
  I also wonder if there is some sensible upper limit
  lowe than 4 billion (as is now the case).

  Also the "May be changed" ?? I wonder if you mean
  "MAY only be changed". or "Can only be changed"
  or "MUST NOT be changed when ... is active"?[[Orly Nicklass]] fixed

- For
   pwTDMCfgSetUp2SynchTimeOut OBJECT-TYPE
     SYNTAX        Unsigned32

  Does a vlaue of zero make sense?[[Orly Nicklass]] not really

- For
   pwTDMCfgAlarmThreshold OBJECT-TYPE
     SYNTAX        Unsigned32
     MAX-ACCESS    read-create
     STATUS        current
     DESCRIPTION
         "Alarms are only reported when the defect state persists
          for the length of time specified by this object.
          The object's unit is millisec.

  maybe add a UNITS clause
  same for pwTDMCfgClearAlarmThreshold[[Orly Nicklass]] done

- FOr
   pwTDMCfgStorageType  OBJECT-TYPE
     SYNTAX            StorageType
     MAX-ACCESS        read-create
     STATUS            current
     DESCRIPTION
         "This variable indicates the storage type for this
          row."
     ::= { pwTDMCfgEntry 19 }

  The DESCRIPTION clause must state if (and if so which) any
  columnds MUST be writeable for 'permanent' entries.

- For
   pwTDMCfgPktFiller OBJECT-TYPE

      SYNTAX        Unsigned32 (0..255)
      MAX-ACCESS    read-create
      STATUS        current
      DESCRIPTION
          "Filler byte pattern played out on the TDM

  Is this really an unsigned32, or would OCTET STRING (SIZE (1)) be
  a better representation. I can live with what you have. I am just
  thinking/wondering aloud.[[Orly Nicklass]] will stay as is

- I can live with this SEQUENCE
   PwTDMCfgEntry ::= SEQUENCE {
        pwTDMCfgIndex                    PwTDMCfgIndex,
        pwTDMCfgRowStatus                RowStatus,
        pwTDMCfgPayloadSize              Unsigned32,
        pwTDMCfgPktReorder               TruthValue,
        pwTDMCfgRtpHdrUsed               TruthValue,
        pwTDMCfgJtrBfrDepth              Unsigned32,
        pwTDMCfgPayloadSuppression       INTEGER,

        pwTDMCfgConsecPktsInSynch        Unsigned32,
        pwTDMCfgConsecMissPktsOutSynch   Unsigned32,
        pwTDMCfgSetUp2SynchTimeOut       Unsigned32,

        pwTDMCfgPktReplacePolicy         INTEGER,

        pwTDMCfgAvePktLossTimeWindow     Integer32,
        pwTDMCfgExcessivePktLossThreshold   Unsigned32,

        pwTDMCfgAlarmThreshold           Unsigned32,
        pwTDMCfgClearAlarmThreshold      Unsigned32,

        pwTDMCfgMissingPktsToSes         Unsigned32,

        pwTDMCfgTimestampMode            INTEGER,
        pwTDMCfgStorageType              StorageType,
        pwTDMCfgPktFiller                Unsigned32,
        pwTDMCfgName                     SnmpAdminString
        }
  But since this is the initial revision of the MIB module,
  would it not make sense to locate the StoragType and
  RowStatus close to each other? Just wondering aloud.
  It would be more consistent with other IETF defined MIB
  modules.[[Orly Nicklass]] will stay as is due to existing
implementations

- For
   pwTDMPerfIntervalDuration OBJECT-TYPE
      SYNTAX      Unsigned32

   Might want to add a UNITS clause

  same for pwTDMPerf1DayIntervalDuration[[Orly Nicklass]] done for both

- For this
   pwTDMGroups      OBJECT IDENTIFIER ::= { pwTDMConformance 1 }
   pwTDMCompliances OBJECT IDENTIFIER ::= { pwTDMConformance 2 }

  I would follow RFC4181 and turn the sequence around, i.e.

   pwTDMCompliances OBJECT IDENTIFIER ::= { pwTDMConformance 1 }
   pwTDMGroups      OBJECT IDENTIFIER ::= { pwTDMConformance 2 }

- In MODULE-COMPLIANCE

                    OBJECT pwGenTDMCfgIndex
                    MIN-ACCESS read-only
                    DESCRIPTION
                        "The ability to set the an index pointer
                        is not required."

  typo: s/the an index/an index/[[Orly Nicklass]] done
  occurs multiple times

  Often we see as DESCRIPTION clause "Write access is not required."
  I like yours somewhat better, cause it is more explicit as
  to what the reault of read-only access is.

Several comments are made once, but apply to other places as well.
Pls do check for similar occurences.

----- admib/bureaucratic/typos etc

- no citations to two referenced documents:
  !! Missing citation for Normative reference:
  P040 L023:    [G.704]      ITU-T Recommendation G.704 (10/98) -
Synchronous frame

  !! Missing citation for Normative reference:
  P040 L027:    [ITU-T-G.826] ITU-T G.826: Error performance parameters
and

- In security Considerations Section, pls s/IPSec/IPsec/
  sec must be all lowercase.
[[Orly Nicklass]] done for the last 3

Bert Wijnen

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

---

From: "Bert Wijnen - IETF" <[email-masked]>
Date: Tue, 18 Dec 2007 16:57:03 +0100

Thanks, will check new draft when I see it.

Bert Wijnen

  -----Oorspronkelijk bericht-----
  Van: Orly Nicklass [mailto:[email-masked]]
  Verzonden: dinsdag 18 december 2007 15:55
  Aan: WIJNEN, Bert (Bert)
  CC: Stewart Bryant; Danny McPherson; [email-masked]; MIB Doctors (E-mail)
  Onderwerp: [MIB-DOCTORS] RE: MIB Doctor review
of:draft-ietf-pwe3-tdm-mib-08.txt

  Orly Nicklass, Ph.D

  Hod Hasharon, Israel

  Tel: +972-9-7751290
  Mobile: +972-54-2209565

----------------------------------------------------------------------------
--

  Pls note that revision 8 has expired. I have reviewed the copy I still had
on my machine.

  Here are my comments.

  The DESCRIPTION clause shows that zerop seem not to be a valid value.
There is probably also a max number of days that makes sense So a syntax of
Unsigned32 (1..30) or some such seems to make sense and be inline with the
range of pwTDMValidDayIntervals.

  By the way, see also my comments on the pwATM MIB module. I personally
think that Unsigned32 is OK, but text in RFC2493 might even want us to use
INTEGER instead of Unsigned32. Maybe some old-time telecom experts can tell
us.

  I see
       ::= { transmission XXX }

  And again I wonder if allocation under transmission is correct/justified.
  You do not want a PW to be an entry in the ifTable. And transmission is
for media-specific MIB data for an interface of a specific ifType.
  SO I think allocation under mib-2 makes more sense.

  [[Orly Nicklass]] done

  I see:
     -- Tables, Scalars

     -- Conformance

     -- Tables, Scalars

  - I see

       DESCRIPTION

  - This one
     pwTDMCfgPayloadSuppression  OBJECT-TYPE
       SYNTAX        INTEGER
                      {
                         enable  ( 1),
                         disable ( 2)
                      }
    Could also be represented with a SYNTAX of TruthValue, for example
       pwTDMCfgEnablePayloadSuppression  OBJECT-TYPE
          SYNTAX        TruthValue
    That would mean re-use of an already existing TC.
    Not a major issue, just better re-use.[[Orly Nicklass]] stays as is due
to existing implementations

  - W.r.t.

    I am not sure if the value zero makes sense (ever)?[[Orly Nicklass]] no

  - For

  - For

    maybe add a UNITS clause

  - FOr

  - For

  - For

  - For this

  - In MODULE-COMPLIANCE

                      OBJECT pwGenTDMCfgIndex
                      MIN-ACCESS read-only
                      DESCRIPTION

  Bert Wijnen

---

From: Thomas Nadeau <[email-masked]>
Date: Wed, 19 Dec 2007 09:06:01 -0500

Thanks.

Happy Holidays all!

	--Tom

On Dec 18, 2007, at 9:55 AM, Orly Nicklass wrote:

> First, I am not subscribed to the WG mailing list, so pls copy me if  
> you want me to see any comments/responses.
>
> Pls note that revision 8 has expired. I have reviewed the copy I  
> still had on my machine.
>
> Here are my comments.
>
>
> The DESCRIPTION clause shows that zerop seem not to be a valid  
> value. There is probably also a max number of days that makes sense  
> So a syntax of Unsigned32 (1..30) or some such seems to make sense  
> and be inline with the range of pwTDMValidDayIntervals.
>
> By the way, see also my comments on the pwATM MIB module. I  
> personally think that Unsigned32 is OK, but text in RFC2493 might  
> even want us to use INTEGER instead of Unsigned32. Maybe some old- 
> time telecom experts can tell us.
>
> You do not want a PW to be an entry in the ifTable. And transmission  
> is for media-specific MIB data for an interface of a specific ifType.
>           if the row index is in used by an entry in pwTDMTable. 
> [[Orly Nicklass]] left in
>   Not a major issue, just better re-use.[[Orly Nicklass]] stays as  
> is due to existing implementations
>
> - W.r.t.
> pwe3 mailing list
> [email-masked]
> https://www1.ietf.org/mailman/listinfo/pwe3