[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PWE3] RE: [MIB-DOCTORS] MIB Doctor review of: draft-ietf-pwe3-tdm-mib-08.txt
Correct. They have not yet defined any NOTOFICATION-TYPEs, so
for now it did/does not matter. Once they start defining any
of those, they would hav have to add another subid (value 0)
to comply with that rule.
Bert Wijnen
> -----Original Message-----
> From: David Harrington [mailto:ietfdbh at comcast.net]
> Sent: Thursday, October 18, 2007 2:26 AM
> To: WIJNEN, Bert (Bert); 'Orly Nicklass'
> Cc: pwe3 at ietf.org; 'MIB Doctors (E-mail)'
> Subject: RE: [MIB-DOCTORS] MIB Doctor review of:
> draft-ietf-pwe3-tdm-mib-08.txt
>
> Hi,
>
> I beleieve Notitifcation = { ... 0 } also handles the issue
> of having a 0 sub-oid to indicate this is not a translation
> ala rfc3584 (if I stated that appropriately).
>
> dbh
>
> > -----Original Message-----
> > From: WIJNEN, Bert (Bert) [mailto:bwijnen at alcatel-lucent.com]
> > Sent: Wednesday, October 17, 2007 10:40 PM
> > To: Orly Nicklass
> > Cc: pwe3 at ietf.org; MIB Doctors (E-mail)
> > Subject: [MIB-DOCTORS] 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
> >
> > 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 }
> >
> >
> > 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 }
> > -- 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.
> >
> >
> > 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 }
> >
> > - 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?
> > Maybe you mean that the row becomes active once/if the base
> > row in the generic pwTable becomes active?
> > Anyway, you want to be cleaerer as to what you mean here.
> > It is better to not use citations (like [PWMIB]) in DESCRIPTION
> > clauses.
> >
> > - 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."
> >
> > - 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.
> > 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.
> >
> > typo: "is in used" shodul be "is in use." I think
> >
> > - 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.
> > 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.
> >
> > - 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.
> >
> > - 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)?
> > 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"?
> >
> > - For
> > pwTDMCfgSetUp2SynchTimeOut OBJECT-TYPE
> > SYNTAX Unsigned32
> >
> > Does a vlaue of zero make sense?
> >
> > - 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
> >
> > - 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.
> >
> > - 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.
> >
> > - For
> > pwTDMPerfIntervalDuration OBJECT-TYPE
> > SYNTAX Unsigned32
> >
> > Might want to add a UNITS clause
> >
> > same for pwTDMPerf1DayIntervalDuration
> >
> > - 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/
> > 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.
> >
> >
> > Bert Wijnen
> >
> >
> > _______________________________________________
> > MIB-DOCTORS mailing list
> > MIB-DOCTORS at ietf.org
> > https://www1.ietf.org/mailman/listinfo/mib-doctors
> >
>
>
>
_______________________________________________
pwe3 mailing list
pwe3 at ietf.org
https://www1.ietf.org/mailman/listinfo/pwe3