RE: My review of: draft-ietf-hubmib-efm-cu-mib-06.txt
"Wijnen, Bert \(Bert\)" <[email protected]> Tue, 20 Feb 2007 17:14:04 +0100
| Newsgroups | gmane.ietf.hubmib |
|---|---|
| Message-ID | <[email protected]> |
Thanks for the revision Ed,
I still have some comments though.
- Not sure if I mentioned this before. But anyway.
I see:
ifCapStackCompliance MODULE-COMPLIANCE
STATUS current
DESCRIPTION
"The compliance statement for SNMP entities, which provide
information on the cross-connect capability of multi-layer
(stacked) network interfaces, with flexible cross-connect
between the sub-layers.
! Compliance with the following external compliance statements
! is REQUIRED:
!
! MIB Module Compliance Statement
! ---------- --------------------
! IF-MIB ifCompliance3
! IF-INVERTED-STACK-MIB ifInvCompliance"
MODULE -- this module
MANDATORY-GROUPS {
ifCapStackGroup
}
OBJECT ifCapStackStatus
SYNTAX TruthValue { true(1) }
DESCRIPTION
"Support for the false(2) value is OPTIONAL for
implementations supporting pluggable interfaces."
OBJECT ifInvCapStackStatus
SYNTAX TruthValue { true(1) }
DESCRIPTION
"Support for the false(2) value is OPTIONAL for
implementations supporting pluggable interfaces."
MODULE IF-MIB
MANDATORY-GROUPS {
ifStackGroup2
}
@ MODULE IF-INVERTED-STACK-MIB
@ MANDATORY-GROUPS {
@ ifInvStackGroup
@ }
::=3D { ifCapStackCompliances 1 }
My concerns are these:
- I wonder if we want to keep (all) the lines that I marked with !
- part of it is already coverd with the lines I marked with @
So I would at least remove the line
IF-INVERTED-STACK-MIB ifInvCompliance
to avoid duplication.=20
At the other hand, I could also live with what you have.
- But I do want you to fix SMICng reported error:
E: f(rfc2864.mi2), (168,26) Item "ifStackGroup2" should be IMPORTed
=20
since you do list that as a mandatory group.
- see below answers from me for resolutions that I am not
agreeing to. I removed all the items we agree on. Thanks
for fixing or answering/explaining.
=20
> > From: Wijnen, Bert (Bert)
> > [mailto:[email protected]]
> > Sent: Thursday, January 18, 2007 21:45
> > To: Edward Beili
> > Cc: Dan Romascanu (E-mail)
> > Subject: RE: [Hubmib] My review of:=20
> > draft-ietf-hubmib-efm-cu-mib-06.txt
>=20
> >- Did we resolve the use of Rowstatus for the ifCapStackTable
> > and ifInvCapStackTable? In any event, pls re-check the
> > feedback we've got on that. I do not think that what we
> > currently have in the MIB module is acceptable.
>=20
> [EB] Replaced with TruthValue.
>=20
This is much better.
I wonder if it would now be better to rename the object from
ifCapStackStatus to ifCapStackCapability to better represent
its purpose. Same for possibly renaming ifInvCapStackStatus
into ifInvCapStackCapability.
I am not hung up on it though.
> >- 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 =20
> > SYNTAX for the discovery code. Can you explain, or point me to the=20
> > 802.3ah clause that explains/justifies that?
>=20
> [EB] The remote discovery code is defined by 802.3ah as a=20
> 6-octets (48-bit) value, see 45.2.6.8. Clause 61A.2 actually=20
> suggests to use a MAC address for the discovery operation, in=20
> order to ensure its 'local uniqueness'.
> Since PhysAddress represents media- or physical-level=20
> addresses it was a natural choice. I've added the size=20
> attribute to indicate it's length.=20
>=20
Would it not be wise to also add 45.2.6.8 and 61A.2 in the
REFERENCE clause?
> >- 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=20
> > value to be ignored/irrelevant? Pls be specific.
> >
> > You use that at various other places as well. Pls check and fix.
>=20
> [EB] Replaced 'unavailable' with 'unsupported'
>=20
But I keep wondering what that means? Does the object get instantiated
with some value that means "unsupported"? What is a SNMP SET occurs,
that one gets rejected I guess (probably with inconsistentValue for
SNMP)?
Or is the object not instantiated?
Or if read, does it return a zero-length OCTET STRING?
Oh no, that is not possible, cause the TC has a SIZE (1..6)
Maybe we need a TC EfmProfileIndexListOrNone, which allows a=20
SIZE (0..6), and requries that an OBJECT of that SYNTAX specifies in
the DESCRIPTION clause what the zero length means. And in this case
it could mean "unsupported".
Similar questions/comments for other uses of "unsupported"
> >- 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,
> > 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=20
> > instantiated. Might be easier (on NM apps) if there was a value
(like=20
> > all zeros, or zerolength string) that can be ignored.
>=20
> [EB] Reworded as follows:
> This object is irrelevant in CPE port (-R) subtypes: in this
> case a zero length octet string SHALL be returned on an
> attempt to read this object, writing to this object SHALL
> be ignored.
MMM... remember I do not understand what "writing to this object SHALL
be ignored" means. You had replaced "ignored" with "rejected" in other
objects. Maybe you better do that here as wel;l.
You also changed the SYNTAX from
SYNTAX PhysAddress
to
SYNTAX PhysAddress (SIZE(6))
Such was needed for INDEX objects, but not necessarily for this object.
I do find it wise though to also be specific as to what the SIZE is,
specifically since the DESCRIPTION clause does state that it is a
6-octet
value. However, that makes it invalid to state:
A zero-length octet string SHALL be returned on an attempt to
read this object when PAF aggregation is not enabled.
This object is irrelevant in CPE port (-R) subtypes: in this
case a zero length octet string SHALL be returned on an
attempt to read this object, writing to this object SHALL
be ignored.
Because the SYNTAX states that it CANNOT be zero length. To cover this,
you
can use a SYNTAX of:
SYNTAX PhysAddress (SIZE(0 | 6))
> >- For efmCuPme2BProfileRowStatus I wonder if any (all?) of the
objects
> > in a row can be changed while the row is active? Would that not
> > be disruptive? In any event, pls specify if any (which( objects
> > can be changed or stat eif none can be change in active state.
>=20
> [EB] I believe that 'read-create' in MAX-ACCESS clause of all=20
> the objects in a row (except for the 'not-accessible' index)=20
> already indicates that in order to change an object in a=20
> particular profile, one would have to create a new one,=20
> change the reference(s) from the old profile to a new one and=20
> remove the old (inactive) profile if not needed anymore.
>=20
Mmm... a MAX-ACCESS read-create does not imply that semantic.
So I would be more specific and state this somewhere.=20
the DESCRIPTION clause of the RowStatus object might be a good=20
place for that.
I could imagine that efmCuPme2BProfileDescr might be changed even
for an active row. At least from the DESCRIPTION clause of that
object, I do not get the impression that it controls/prescribes
operational attributes, does it?
I can see that for the other objects, they probably cannot be=20
changed while the row is active and referenced. But maybe one
could put the row in notInService, changed the attributes and
use it again?
I hope I am now explaining better why I wondered (and still wonder).
In any event, this is not clear from the current definitions.
If you want to prescirbe that=20
in order to change an object in a particular profile,=20
one would have to create a new one, change the reference(s)
from the old profile to a new one and remove the old=20
(inactive) profile if not needed anymore.
then pls be explict and put it in a DESCRIPTION clause.
>=20
> > Same question for efmCuPme2BsModeRowStatus, although there a change
> > is probably not disruptive.
> > Pls check all occurences of RowStatus
>=20
> [EB] Same answer as above.
>=20
Same comment as above.
> >- I really wonder if we are doing a smart thing by give the=20
> 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?
>=20
> [EB] If we take it to extreme why not turning all enums to TCs?
> Personally I prefer inline enums (if they are not repeated of=20
> course which would make they perfectly good candidates for=20
> TCs) as they list the values and their meaning in the=20
> description for the object - less jumps between the object=20
> and a TC when reading the MIB.
>=20
As I said. I wondered if it is smart.
If no one else wonders, and thereby (implicitly, by being silent)
agrees with you. Then I am OK with what you have.
> >- 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.
>=20
> [EB] While I tend to agree with the advantages of the naming=20
> scheme you proposed, I would like to ask to be forgiven and=20
> leave the names as they are now, due to the following rasons:
> - It is a big change (considering also that there are at=20
> least 2 experimental implementations of this draft that I know of)
I know it is a big change, and so I am willing to accept what you=20
have because of that.
The fact that there are 2 experimental implementations is somewhat
of a bad argument. They should (in order to avoid conflicts
with whatever we finally produce as a standards track MIB module)
have taken an approach whereby:
- they have rooted the MIB module under their own enterprise OID
tree
- renamed the MIB mdoule and all objects so they won't conflict=20
with our (soon to be) standard MIB module. Normally one then
prefixes all object names with the company name or some such.
> - Some time ago I was asked to stick with the RFC 2578/2579=20
> recommendation of 32 char restriction - the proposed change=20
> would make some names to be longer than 32 chars. (Yes, I=20
> just read RFC 4181 view on that - the 32 character=20
> restriction recommendation of SMIv2 SHOULD be set aside in=20
> favor of promoting clarity and uniqueness). However it could=20
> be argued that it is easier to remember=20
> efmCuLowRateCrossingEnable vs. efmCuPortConfLowRateCrossingEnable
>=20
Not so strong argument, but I understand what you are saying.
So based on the "it is a BIG change and it is late in the process"
I will not require the change to be made. Hope Dan agrees as well,
and I assume that WG members also agree.=20
WG members/paritcipants: Speak up if not!
> >- 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 =20
> > DESCRIPTION clause is not needed. In general people expect=20
> (I think) =20
> > writable objects when they see such a ststament.
>=20
> [EB] You are right. However when one is looking at the=20
> description clause of a 'not-accessible' table, the only way=20
> to determine if it is composed of read-only objects is to=20
> check the Max access value of each and every object in the=20
> table. I believe that the 'NOT persistent'
> sentence helps reader to understand the nature of a table=20
> quicker, so I would leave it as is.
>=20
Mmmm... also read-write objects can be NON-persistent (volatile).
So I do not follow your logic ??
I believe I still see other objects that are read-write or read-create
and for which it is not clear what the persistency behaviour is=20
supposed to be. Can you pls check and make sure that that is specified
for all writable objects. If the behaviour is the same for all
objects in a row, you can describe it in the xxxEntry DESCRITPION
clause for that row.
Bert