RE: My review of: draft-ietf-hubmib-efm-cu-mib-06.txt
"Wijnen, Bert \(Bert\)" <[email protected]> Thu, 18 Jan 2007 20:39:50 +0100
| Newsgroups | gmane.ietf.hubmib |
|---|---|
| Message-ID | <[email protected]> |
My appology, "this week" took a bit longer than I
had expected.=20
So here is my review as WG chair.
- SMICng:
W: f(EFM-CU-MIB.mi2), (300,21) Item "efmCuPAFDiscoveryCode" should
have SIZE specified
W: f(EFM-CU-MIB.mi2), (1178,21) Item "efmCuPAFRemoteDiscoveryCode"
should have SIZE specified
W: f(EFM-CU-MIB.mi2), (2925,23) For "efmCuPmeSubTypesSupported",
syntax is identical
for the first two, I think: SIZE(6)
for the 3rd one I am OK to let the warning go., i.e. ignore it.
W: f(IF-CAP-STACK-MIB.mi2), (195,17) Row "ifInvCapStackEntry" does not
have a consistent indexing scheme - index items must be in same
order
as used in INDEX clause for "base row" ifStackEntry
E: f(IF-CAP-STACK-MIB.mi2), (291,13) Item "ifInvStackGroup" should be
IMPORTed
Warning is OK I think, but pls fix error.
- You may want to add a note-to-rfcs-editor to:
[I-D.ietf-hubmib-efm-mib]
Squire, M., "Definitions and Managed Objects for OAM
Functions on Ethernet Like Interfaces",
draft-ietf-hubmib-efm-mib-04 (work in progress),
March 2006.
[I-D.ietf-hubmib-rfc3636bis]
Beili, E., "Definitions of Managed Objects for IEEE 802.3
Medium Attachment Units (MAUs)",
draft-ietf-hubmib-rfc3636bis-05 (work in progress),
July 2006.
in which you ask them to replace with actual RFC numbers if those
are available at time of publication.
- note that if you do a revision, you must use new copyright
and no longer=20
Copyright (C) The Internet Society (2006).
See my earlier post to the hubmib list
- Did we resolve the use of Rowstatus for the ifCapStackTable
and ifInvCapStackTable? In any event, pls re-check the=20
feedback we've got on that. I do not think that what we
currently have in the MIB module is acceptable.
- In order to avoid any possible future name clashed, I would
prefer it if we rename Textual Convnetions:
ProfileIndex ::=3D TEXTUAL-CONVENTION
ProfileIndexOrZero ::=3D TEXTUAL-CONVENTION
ProfileIndexList ::=3D TEXTUAL-CONVENTION
TruthValueOrUnknown ::=3D TEXTUAL-CONVENTION
such that they are prefixed with Efm, so I would name them:
EfmProfileIndex ::=3D TEXTUAL-CONVENTION
EfmProfileIndexOrZero ::=3D TEXTUAL-CONVENTION
EfmProfileIndexList ::=3D TEXTUAL-CONVENTION
EfmTruthValueOrUnknown ::=3D TEXTUAL-CONVENTION
this is also common practice in most (if not all) other=20
IETF MIB modules these days
- for efmCuPAFAdminState you describe a few operations that
should be ignore when tried. I am not sure how to inerpret
that and what would happen when a SNMP SET is received.
I would think it is better to say the need to be rejected.
(that is, cause an error on an SNMP SET).
- for=20
efmCuPAFDiscoveryCode OBJECT-TYPE
SYNTAX PhysAddress
MAX-ACCESS read-write
STATUS current
DESCRIPTION
"PAF Discovery Code of the EFMCu port (PCS).
A unique 6 Byte long code used by the Discovery function, when
PAF is supported.
PCS ports incapable of supporting PAF SHALL return a value of
all zeroes. Attempts to change this object SHALL be ignored in
this case.
This object MUST be instantiated for the -O subtype PCS before
writing operations on the efmCuPAFRemoteDiscoveryCode
(Set_if_Clear and Clear_if_Same) are performed by PMEs
associated with the PCS.
The value of this object is read-only for -R port subtypes.
The initial value of this object for -R ports after reset
is 0. This value may be changed as a result of writing
operation on efmCuPAFRemoteDiscoveryCode variable of remote
PME of -O subtype, connected to one of the local PMEs
associated with the PCS.
Discovery MUST be performed when the link is Down.
Attempts to change this object MUST be rejected with the error
inconsistentValue if the link is Up or Initializing.
The PAF Discovery code maps to the local Discovery code
variable in PAF (note that it does not have a corresponding
Clause 45 register)"
REFERENCE
"[802.3ah] 61.2.2.8.3, 61.2.2.8.4, 45.2.6.6.1"
::=3D { efmCuPortConfEntry 2 }
I am trying to figure out what made you choose PhysAddress as the
SYNTAX for the discovery code. Can you explain, or point me to
the 802.3ah clause that explains/justifies that?
I wonder what you mean with "the value of this object is read-only"
The value has to be a PhysAddress ( 6 octets) value, no?
If you mean that for -R port subtypes the object cannot allow write
access, then I would say it in terms aka:
The initial value of this object for -R port subtypes after
reset is all zeroes. For such -R ports, the value of this
object cannot be changed directly. The value may be changed
as a result of a writing operation on the=20
efmCuPAFRemoteDiscoveryCode object of a remote PME of=20
-O subtype, connected to one of the local PMEs
associated with the PCS.
I hope I understood the intention and reworded it properly.
These day we prefer to not hard-wire SNMP error codes in DESCRIPTION
clause, or if we do, then as an example (A MIB could be used by non
SNMP protocols). SO I suggest=20
=20
Discovery MUST be performed when the link is Down.
Attempts to change this object MUST be rejected (in case of
SNMP with the error inconsistentValue) if the link is Up=20
or Initializing.
- For efmCuAdminProfile I read in DESCRIPTION clause:
This object is writable and readable for the -O subtype
(2BaseTL-O or 10PassTS-O) EFMCu ports. It is unavailable for
the -R subtype (2BaseTL-R or 10PassTS-R) ports.
what does it mean that this object is "unavailable"
Do you not want it instatntiated in that case, or do you want
its value to be ignored/irrelevant? Pls be specific.
You use that at various other places as well. Pls check and fix.
- For
efmCuPAFRemoteDiscoveryCode OBJECT-TYPE
SYNTAX PhysAddress
MAX-ACCESS read-write
STATUS current
DESCRIPTION
"PAF Remote Discovery Code of the PME port at CO.
A 6 Byte long Discovery Code of the peer PCS connected via
the PME.
Reading this object results in a Discovery Get operation.
Writing a zero to this object results in a Discovery
Clear_if_Same operation (the value of efmCuPAFDiscoveryCode
at the peer PCS SHALL be the same as efmCuPAFDiscoveryCode of
the local PCS associated with the PME for the operation to
succeed).
Writing a non-zero value to this object results in a
Discovery Set_if_Clear operation.
This object does not exist in CPE port subtypes. A zero length
octet string SHALL be returned for CPE port subtypes and also
when PAF aggregation is not enabled.
What is "writing a zero value" ??
Is that 6 octets, all zeroes?
Or one octet conatining zero,=20
or the zero length octet-string?
What means "does not exist" is it not instanctiated? I guess so.
pls be clear. You do get gaps in the tables if some objects are
not instantiated. Might be easier (on NM apps) if there was
a value (like all zeros, or zerolength string) that can be ignored.
- I believe I have seen this SYNTAX
SYNTAX INTEGER {
ieee2BaseTLO(1),
ieee2BaseTLR(2),
ieee10PassTSO(3),
ieee10PassTSR(4)
}
several times. Candidate for a Textual Convention?
- for efmCuPme2BRegion I wonder if we ever (soon?) expect more
regions? If so, maybe we ought to make this a TC?
How will regions be added?
- For efmCuPme2BProfileRowStatus I wonder if any (all?) of the objects
in a row can be changed while the row is active? Would that not=20
be disruptive? In any event, pls specify if any (which( objects
can be changed or stat eif none can be change in active state.
Same question for efmCuPme2BsModeRowStatus, although there a change
is probably not disruptive.
Pls check all occurences of RowStatus
- I really wonder if we are doing a smart thing by give the same labels
to the various enums in
efmCuPme10PBandplanPSDMskProfile INTEGER,
efmCuPme10PUPBOReferenceProfile INTEGER,
efmCuPme10PBandNotchProfiles BITS,
efmCuPme10PPayloadURateProfile INTEGER,
efmCuPme10PPayloadDRateProfile INTEGER,
They have different meanings, don't they? So would it not be better
to differentiate between the labels of the neumerations?
- For OBBJECT-GROUP definitions, one is NOT supposed to state
requirements
or optional attributes. Such things (if a group if required or
optional)
are stated in the MODULE-COMPLIANCE.
The OBJECT0GROUP macro is ONLY intended to group objects that
logically
fit together in order to model something.
Sof (for example):
efmCuBasicGroup OBJECT-GROUP
OBJECTS {
efmCuPAFSupported,
efmCuAdminProfile,
efmCuTargetDataRate,
efmCuTargetSnrMgn,
efmCuAdaptiveSpectra,
efmCuPortSide,
efmCuFltStatus
}
STATUS current
DESCRIPTION
"A collection of objects required for all of EFMCu ports."
::=3D { efmCuGroups 1 }
Sould NOT state that these objects are "required". I would phrase
it as:
"A collection of objects reperesenting management information
that is common for all of EFMCu ports."
The fact that is is REQUIRED for all EFMCu ports is expressed in
efmCuCompliance MODULE-COMPLIANCE
STATUS current
DESCRIPTION
... snip ...
MODULE -- this module
MANDATORY-GROUPS {
efmCuBasicGroup,
That is, here it is listed in the MANDATORY-GROUPS clause and so that
inidicated that it is required.
=20
Pls try to rephrase for all OBJECT-GROUPs
- With regard to naming, I am somewhat worried about the conventions
that
are used (or maybe those that are not used). I always find it smarter
to have all columnar objects in a table prefixed with a string that
makes it clear to which table the object belongs. For example for
the efmCuPortConfTable:
EfmCuPortConfEntry ::=3D
SEQUENCE {
efmCuPAFAdminState INTEGER,
efmCuPAFDiscoveryCode PhysAddress,
efmCuAdminProfile ProfileIndexList,
efmCuTargetDataRate Unsigned32,
efmCuTargetSnrMgn Unsigned32,
efmCuAdaptiveSpectra TruthValue,
efmCuThreshLowRate Unsigned32,
efmCuLowRateCrossingEnable TruthValue
}
=20
I would personally prefer the columns to be alll prefixed with=20
efmCuPortConf, so I would have named them as follows:
EfmCuPortConfEntry ::=3D
SEQUENCE {
efmCuPortConfPAFAdminState INTEGER,
efmCuPortConfPAFDiscoveryCode PhysAddress,
efmCuPortConfAdminProfile ProfileIndexList,
efmCuPortConfTargetDataRate Unsigned32,
efmCuPortConfTargetSnrMgn Unsigned32,
efmCuPortConfAdaptiveSpectra TruthValue,
efmCuPortConfThreshLowRate Unsigned32,
efmCuPortConfLowRateCrossingEnable TruthValue
}
This also holds true for most (all of) the other tables.
My suggested naming convention above has (in my view) 2 advantages:
=20
- less change for conflicting names.=20
I will admit, that this is not too big a risk, since this is
all within the same MIB module. Nevertheless, I think it just
makes it easier if any additions need to be made in the future.
- easier for people to see which objects belong to which tables.
I think this is a strong point. But I also understand it is
somewhat subjective.
I can accept if the WG and/or editor/author tells me that I am far
too late with this type of comment. It of course requires a quite=20
massive editorial change. So, although I think it would improve
the human-friendlyness of the MIB module, I will not insist on
this change.
NITS:
- the RFC editor probably wants you to expand acronyms when they are
used for the first time. Pls check you do it for all acronyms.
I found for example VDSL, DMT, SHDSL acronyms which have not
been expanded.
There may be others. Pls check.
- For:
ifCapStackEntry OBJECT-TYPE
SYNTAX IfCapStackEntry
MAX-ACCESS not-accessible
STATUS current
DESCRIPTION
"Information on a particular relationship between two
sub-layers, specifying that one sub-layer runs on 'top' of the
other sub-layer. Each sub-layer corresponds to a conceptual
I wonder if we should: s/runs on top/can run on 'top'/
After all, it is a capability, which not necessarily is used, right?
- for efmCuPAFDiscoveryCode you speak about a "6 Byte code"
If this is common 802.3 terminology, then I guess it is OK.
I know that some people in the IETF prefer to use 'octet' instead
of 'byte'.
- This table
efmCuPmeStatusTable OBJECT-TYPE
SYNTAX SEQUENCE OF EfmCuPmeStatusEntry
MAX-ACCESS not-accessible
STATUS current
DESCRIPTION
"This table provides common status information of EFMCu
2BASE-TL/10PASS-TS PME ports. Status information specific
to 10PASS-TS PME is represented in efmCuPme10PStatusTable.
This table contains live data from the equipment. As such,
it is NOT persistent."
::=3D { efmCuPme 3 }
Is cmposed of just read-only objects. So the last sentence of the
DESCRIPTION clause is not needed. In general people expect (I think)
writable objects when they see such a ststament.
- For efmCuPme2BProfileDescr I think I would change the uppercase MAY
to a lowercase may.
Same for efmCuPme10PProfileDescr.
There may be other places. Pls check RFC2119 to understand how/when
capilaized terms are needed. They are for COMPLIANCE, the two
objects above have no special compliance requirments for content
except for adhering to the SYNTAX, right?
TYPOs:
- typo in table 1:
| ifIndex | Interface index. Note that each PME and each PCS |
| | in the EFMCu PHY MUST have a unique index, as |
| | there some PCS and PME specific attributes |
| | accessible only on the PCS or PME level. |
=20
s/there some/there are some/ I think
- 3rd para of sect 4.3:
A specific configuration or administrative profile is assigned to a
specific PME via efmCuPmeAdminProfile object. If
efmCuPmeAdminProfile is zero, then efmCuAdminProfile object of the
PCS port, connected to the PME, determines the configuration profile
(or a list of possible profiles) for that PME. This mechanism allows
to specify a common profile(s) for all PMEs connected to the PCS
port, with an ability to change individual PME profiles by setting
efmCuPmeAdminProfile object, which overwrites profile set by
efmCuAdminProfile.
s/overwrites profile set by/overwrites the profile set by/
I think.
> -----Original Message-----
> From: Wijnen, Bert (Bert)=20
> Sent: maandag 27 november 2006 16:52
> To: Edward Beili; Wijnen, Bert (Bert)
> Cc: Dan Romascanu (E-mail)
> Subject: RE: [Hubmib] My review of:=20
> draft-ietf-hubmib-efm-cu-mib-06.txt
>=20
> Better to wait. I will try to get my review done this week.
>=20
> Bert
>=20
> > -----Original Message-----
> > From: Edward Beili [mailto:[email protected]]
> > Sent: Monday, November 27, 2006 16:41
> > To: Wijnen, Bert (Bert)
> > Cc: Dan Romascanu (E-mail)
> > Subject: RE: [Hubmib] My review of:=20
> > draft-ietf-hubmib-efm-cu-mib-06.txt
> >=20
> >=20
> > Bert,
> > I've got a few comments about IF-CAP-STACK-MIB in addition to yours.
> > Should I issue another version of
> > draft-ietf-hubmib-efm-cu-mib fixing those or should I wait for you=20
> > comments on the EFM-CU-MIB?
> >=20
> > Regards,
> > -E.
> >=20
> > > -----Original Message-----
> > > From: Wijnen, Bert (Bert) [mailto:[email protected]]
> > > Sent: Wednesday, October 18, 2006 5:45 PM
> > > To: Hubmib Mailing List (E-mail)
> > > Subject: [Hubmib] My review of:=20
> draft-ietf-hubmib-efm-cu-mib-06.txt
> > >=20
> > >=20
> > > My WG LC review comments are here.
> > > For the EFM-U-MIB itself I will do a separate posting to the list.
> > >=20
> > > - Abstract 3rd line
> > >=20
> > > This document proposes an extension to the Ethernet-like=20
> > > Interfaces
> > >=20
> > > By the time we get published as RFC, we no longer "propose".
> > > So how about:
> > >=20
> > > This document describes extensions to the Ethernet-like
> > Interfaces
> > >=20
> > > - Page 7, a nit:
> > > // pottentially be connected to the PCS
> > > s/pottentially/potentially/
> > > // for PCS[i] and there room for another=20
> PME in the
> > > s/there room/there is room/ ??
> > >=20
> > > - A nit/typo on page 10:
> > > | ifIndex | Interface index. Note that each PME and=20
> > > each PCS |
> > > | | in the EFMCu PHY MUST have a unique=20
> > > index, as |
> > > | | there some PCS and PME specific=20
> > > attributes |
> > > | | accessible only on the PCS or PME level.=20
> > > |
> > >=20
> > > s/as there some/as there are some/ ??
> > >=20
> > > - a type in 2nd para in sect 4.2
> > >=20
> > > The PME profiles are defined in efmCuPme2BProfileTable and
> > > efmCu10PProfileTable for 2BASE-TL and 10PASS-TS PMEs
> > respectively.
> > >=20
> > > I think: s/efmCu10PProfileTable/efmCuPme10PProfileTable/
> > > So insert "Pme"
> > >=20
> > > - SMICng IF-CAP-STACK-MIB tells me:
> > >=20
> > > W: f(IF-CAP-STACK-MIB.mi2), (195,17) Row "ifInvCapStackEntry"
> > > does not have a consistent indexing scheme - index=20
> items must
> > > be in same order as used in INDEX clause for "base row"
> > > ifStackEntry
> > >=20
> > > The above is OK, it is an INVERTED table.
> > > So we can ignore the warning.
> > >=20
> > > E: f(IF-CAP-STACK-MIB.mi2), (291,13) Item "ifInvStackGroup"
> > > should be IMPORTed
> > >=20
> > > I think that error should be fixed and we shoul dimport
> > that group.
> > >=20
> > > - SMICNg EFM-CU-MIB tells me:
> > >=20
> > > W: f(EFM-CU-MIB.mi2), (300,21) Item
> > "efmCuPAFDiscoveryCode" should
> > > have SIZE specified
> > >=20
> > > Do we know a reasonable size for this? In the pseudo code on=20
> > > pages 7/8
> > > it speaks about a 6-byte (octet) code. And so does the
> > DESCRIPTION
> > > clause. Is it always fixed to 6 octets?
> > > Now and in future? If so, I would suggest to use
> > >=20
> > > SYNTAX PhysAddress (SIZE(6))
> > >=20
> > > W: f(EFM-CU-MIB.mi2), (1178,21) Item
> > "efmCuPAFRemoteDiscoveryCode"
> > > should have SIZE specified
> > >=20
> > > Same story for the above.
> > >=20
> > > W: f(EFM-CU-MIB.mi2), (2925,23) For=20
> "efmCuPmeSubTypesSupported",
> > > syntax is identical
> > >=20
> > > This is probably OK, although it looks a bit weird:
> > > OBJECT efmCuPmeSubTypesSupported
> > > SYNTAX BITS {
> > > ieee2BaseTLO(0),
> > > ieee2BaseTLR(1),
> > > ieee10PassTSO(2),
> > > ieee10PassTSR(3)
> > > }
> > > DESCRIPTION
> > > "Support for all subtypes is not required.=20
> > > However at least
> > > one value SHALL be supported"
> > >=20
> > > - W.r.t. the SYNTAX of objects ifCapStackStatus and=20
> > > ifInvCapStackStatus
> > > I think it would be much better to use a SYNTAX of TruthValue.
> > > See my separate posting on this topic to the HubMIB WG list.
> > >=20
> > > - I believe that instead of
> > >=20
> > > ifCapStackConformance OBJECT IDENTIFIER
> > > ::=3D { ifCapStackObjects 3 }
> > >=20
> > > It would be better to adhere to the strcuture suggested by
> > > RFC4181 page 38
> > > and so use instead:
> > >=20
> > > ifCapStackConformance OBJECT IDENTIFIER ::=3D {
> > ifCapStackMIB 2 }
> > >=20
> > > - I still need to review the EFM-CU-MIB module. I will do=20
> a separate=20
> > > posting
> > > on that one once I am done.
> > >=20
> > > - In the Security Considerations section I see capitalized MAY
> > > which I think is not what RFC2119 intended. Unless you can=20
> > > explain to me why this capilaized MAY makes sense, I would
> > > prefer if we change it to just lowercase "may" for all
> > > occurences in the Security Considerations section.
> > >=20
> > > - 2nd para on page 83 (a NIT):
> > > Even if the network itself is secure (for example by
> > using IPSec),
> > > pls change "IPSec" into "IPsec", that is a lower case "s".
> > > The current MIB security template has it fixed. I know that the
> > > Security ADs want/prefer the proper spelling.
> > >=20
> > > - IANA Considerations.
> > > Since you state that some values SHALL be defined in the
> > > IANA-MAU-MIB, I think that you make rfc3636bis a normative
> > > reference, while it is now listed under informative.=20
> And by making
> > > that document normative, you/we automagically make sure=20
> that this
> > > efmCuMIB will not get published before 3636bis.
> > >=20
> > > You must also request/ask (in IANA COnsiderations section) that=20
> > > IANA assigns two new OID branches for
> > >=20
> > > ifCapStackMIB ::=3D { mib-2 ZZZ }
> > >=20
> > > efmCuMIB ::=3D { mib-2 YYY }
> > >=20
> > > - If/wehen we do a revision, we must adhere to new boilerplate.
> > > That means we must change all occurences of:
> > >=20
> > > Copyright (C) The Internet Society (2006).
> > >=20
> > > into
> > >=20
> > > Copyright (C) The IETF Trust (2006).
> > >=20
> > > If you are using xml2rfc, thsi can be achieved with:
> > >=20
> > > ipr=3D"full3978update"
> > >=20
> > > You can/could not yet know this. I know that this starts
> > on Nov 1st
> > > (because I was asked to update the IDChecklist.html.
> > >=20
> > > Bert
> > >=20
> > > _______________________________________________
> > > Hubmib mailing list
> > > [email protected]
> > > https://www1.ietf.org/mailman/listinfo/hubmib
> > >=20
> >=20
>=20