RE: [Hubmib] My review of: draft-ietf-hubmib-efm-cu-mib-06.txt

["Wijnen, Bert \(Bert\)" <[e-mail]>]

My appology, "this week" took a bit longer than I
had expected. 

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 
   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 
  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 ::= TEXTUAL-CONVENTION
     ProfileIndexOrZero ::= TEXTUAL-CONVENTION
     ProfileIndexList ::= TEXTUAL-CONVENTION
     TruthValueOrUnknown ::= TEXTUAL-CONVENTION

  such that they are prefixed with Efm, so I would name them:


     EfmProfileIndex ::= TEXTUAL-CONVENTION
     EfmProfileIndexOrZero ::= TEXTUAL-CONVENTION
     EfmProfileIndexList ::= TEXTUAL-CONVENTION
     EfmTruthValueOrUnknown ::= TEXTUAL-CONVENTION

  this is also common practice in most (if not all) other 
  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 
     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"
       ::= { 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 
         efmCuPAFRemoteDiscoveryCode object of a remote PME of 
         -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 
  
         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 
         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, 
  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 
  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."
        ::= { 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.
  
  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 ::=
       SEQUENCE {
         efmCuPAFAdminState               INTEGER,
         efmCuPAFDiscoveryCode            PhysAddress,
         efmCuAdminProfile                ProfileIndexList,
         efmCuTargetDataRate              Unsigned32,
         efmCuTargetSnrMgn                Unsigned32,
         efmCuAdaptiveSpectra             TruthValue,
         efmCuThreshLowRate               Unsigned32,
         efmCuLowRateCrossingEnable       TruthValue
       }
  
  I would personally prefer the columns to be alll prefixed with 
  efmCuPortConf, so I would have named them as follows:

     EfmCuPortConfEntry ::=
       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:
  
    - less change for conflicting names. 
      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 
   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."
       ::= { 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.          |
 
  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) 
> Sent: maandag 27 november 2006 16:52
> To: Edward Beili; Wijnen, Bert (Bert)
> Cc: Dan Romascanu (E-mail)
> Subject: RE: [Hubmib] My review of: 
> draft-ietf-hubmib-efm-cu-mib-06.txt
> 
> Better to wait. I will try to get my review done this week.
> 
> Bert
> 
> > -----Original Message-----
> > From: Edward Beili [mailto:[e-mail]]
> > Sent: Monday, November 27, 2006 16:41
> > To: Wijnen, Bert (Bert)
> > Cc: Dan Romascanu (E-mail)
> > Subject: RE: [Hubmib] My review of: 
> > draft-ietf-hubmib-efm-cu-mib-06.txt
> > 
> > 
> > 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 
> > comments on the EFM-CU-MIB?
> > 
> > Regards,
> > -E.
> > 
> > > -----Original Message-----
> > > From: Wijnen, Bert (Bert) [mailto:[e-mail]]
> > > Sent: Wednesday, October 18, 2006 5:45 PM
> > > To: Hubmib Mailing List (E-mail)
> > > Subject: [Hubmib] My review of: 
> draft-ietf-hubmib-efm-cu-mib-06.txt
> > > 
> > > 
> > > My WG LC review comments are here.
> > > For the EFM-U-MIB itself I will do a separate posting to the list.
> > > 
> > > - Abstract 3rd line
> > > 
> > >     This document proposes an extension to the Ethernet-like 
> > > Interfaces
> > > 
> > >   By the time we get published as RFC, we no longer "propose".
> > >   So how about:
> > > 
> > >     This document describes extensions to the Ethernet-like
> > Interfaces
> > > 
> > > - Page 7, a nit:
> > >            // pottentially be connected to the PCS
> > >   s/pottentially/potentially/
> > >                  // for PCS[i] and there room for another 
> PME in the
> > >   s/there room/there is room/ ??
> > > 
> > > - A nit/typo on page 10:
> > >    | 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. 
> > >          |
> > > 
> > >   s/as there some/as there are some/ ??
> > > 
> > > - a type in 2nd para in sect 4.2
> > > 
> > >    The PME profiles are defined in efmCuPme2BProfileTable and
> > >    efmCu10PProfileTable for 2BASE-TL and 10PASS-TS PMEs
> > respectively.
> > > 
> > >   I think: s/efmCu10PProfileTable/efmCuPme10PProfileTable/
> > >   So insert "Pme"
> > > 
> > > - SMICng IF-CAP-STACK-MIB tells me:
> > > 
> > >     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
> > > 
> > >   The above is OK, it is an INVERTED table.
> > >   So we can ignore the warning.
> > > 
> > >     E: f(IF-CAP-STACK-MIB.mi2), (291,13) Item "ifInvStackGroup"
> > >        should be IMPORTed
> > > 
> > >   I think that error should be fixed and we shoul dimport
> > that group.
> > > 
> > > - SMICNg EFM-CU-MIB tells me:
> > > 
> > >     W: f(EFM-CU-MIB.mi2), (300,21) Item
> > "efmCuPAFDiscoveryCode" should
> > >        have SIZE specified
> > > 
> > >    Do we know a reasonable size for this? In the pseudo code on 
> > > 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
> > > 
> > >         SYNTAX      PhysAddress (SIZE(6))
> > > 
> > >     W: f(EFM-CU-MIB.mi2), (1178,21) Item
> > "efmCuPAFRemoteDiscoveryCode"
> > >        should have SIZE specified
> > > 
> > >    Same story for the above.
> > > 
> > >     W: f(EFM-CU-MIB.mi2), (2925,23) For 
> "efmCuPmeSubTypesSupported",
> > >        syntax is identical
> > > 
> > >    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. 
> > > However at least
> > >             one value SHALL be supported"
> > > 
> > > - W.r.t. the SYNTAX of objects ifCapStackStatus and 
> > > 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.
> > > 
> > > - I believe that instead of
> > > 
> > >       ifCapStackConformance OBJECT IDENTIFIER
> > >       ::= { ifCapStackObjects 3 }
> > > 
> > >   It would be better to adhere to the strcuture suggested by
> > > RFC4181 page 38
> > >   and so use instead:
> > > 
> > >      ifCapStackConformance  OBJECT IDENTIFIER ::= {
> > ifCapStackMIB 2 }
> > > 
> > > - I still need to review the EFM-CU-MIB module. I will do 
> a separate 
> > > posting
> > >   on that one once I am done.
> > > 
> > > - In the Security Considerations section I see capitalized MAY
> > >   which I think is not what RFC2119 intended. Unless you can 
> > >   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.
> > > 
> > > - 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.
> > > 
> > > - 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. 
> And by making
> > >   that document normative, you/we automagically make sure 
> that this
> > >   efmCuMIB will not get published before 3636bis.
> > > 
> > >   You must also request/ask (in IANA COnsiderations section) that 
> > >   IANA assigns two new OID branches for
> > > 
> > >      ifCapStackMIB ::= { mib-2 ZZZ }
> > > 
> > >      efmCuMIB  ::= { mib-2 YYY }
> > > 
> > > - If/wehen we do a revision, we must adhere to new boilerplate.
> > >   That means we must change all occurences of:
> > > 
> > >      Copyright (C) The Internet Society (2006).
> > > 
> > >   into
> > > 
> > >      Copyright (C) The IETF Trust (2006).
> > > 
> > >   If you are using xml2rfc, thsi can be achieved with:
> > > 
> > >         ipr="full3978update"
> > > 
> > >   You can/could not yet know this. I know that this starts
> > on Nov 1st
> > >   (because I was asked to update the IDChecklist.html.
> > > 
> > > Bert
> > > 
> > > _______________________________________________
> > > Hubmib mailing list
> > > [e-mail]
> > > https://www1.ietf.org/mailman/listinfo/hubmib
> > > 
> > 
> 

_______________________________________________
Hubmib mailing list
[e-mail]
https://www1.ietf.org/mailman/listinfo/hubmib