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

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

  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
_______________________________________________
pwe3 mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/pwe3

---

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

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.