AD review: draft-ietf-adslmib-adsl2-06.txt

Thanks for revision 6.

I think it is fine to issue an IETF Last Call now.
And then consider the remaining comments I have below
as initial IETF LC comments. You can then do a new rev
at the end of IETF LC or possibly wait to see if IESG
has any firtehr comments and address those at the same
time. I will leave it up to Dan how he wants to handle 
that.

If you all (Authors, WG chair and Dan) agree, then 
I will request IETF LC this week.

FOr remining comments see inline below

W.r.t.
> 
> - I see:
>    Adsl2ConfPmsForce ::= TEXTUAL-CONVENTION
>       STATUS      current
>       DESCRIPTION
>          "Attributes with this syntax are configuration parameters
>           that reference the desired power management state for the
>           ADSL/ADSL2 or ADSL2+ link:
>             l3toL0 (1)         - Perform a transition from L3 to L0
>                                  (Full power management state)
> 
>             l0toL2 (2)         - Perform a transition from L0 to L2
>                                  (Low power management state)
>             l0orL2toL3 (3)     - Perform a transition into L3 (Idle
>                                  power management state)"
> 
>       SYNTAX      INTEGER {
>                      l3toL0 (0),
>                      l0toL2 (2),
>                      l0orL2toL3 (3)
>                   }
>    It seems the actual INTEGER values are out of sync with 
>    the DESCRIPTION.
>    l3toL0 (0) should probably be: l3toL0 (1) !!??
> 

I see that you made it zero, so enumeration 1 is not assigned.
That is OK if there is a good reason: From RFC2578:
  The INTEGER type (but not the Integer32 type) may also be used to
  represent integer-valued information as named-number enumerations.
  In this case, only those named-numbers so enumerated may be present
  as a value.  Note that although it is recommended that enumerated
  values start at 1 and be numbered contiguously, any valid value for
  Integer32 is allowed for an enumerated value and, further, enumerated
  values needn't be contiguously assigned.
As long as you do this consciously, I am OK.

> - In the TC MIB module I see various INTEGER enumerations 
>   that start at 
>   value zero. In general we prefer to start at 1, except if 
>   there is a good
>   reason to use zero (like the fact that the label/value map 
>   directly to a
>   zero value in some protocol or other specification. May I 
>   assume that 
>   such is the case for all INTEGER enumerations that start at zero?
> 
SO I assume all cases have been consciously evaluated and decided upon?


> -    Adsl2ScMaskUs ::= TEXTUAL-CONVENTION
>       STATUS      current
>       DESCRIPTION
> 
>          "Each one of the 64 bits in this OCTET
>          STRING array represents the corresponding bin
>          in the downstream direction.  A value of one
>          indicates that the bin is not in use."
>       SYNTAX      OCTET STRING (SIZE(0..8))
>    Most places where you use "Us" in the object descriptor, it is
>    for UpStream. So is this ...MarkUs indeed for downstream?
> 
> In the adsl2MIB module:
> 
> -    adsl2LineCnfgTemplate  OBJECT-TYPE
>       SYNTAX      SnmpAdminString (SIZE(1..32))
>       MAX-ACCESS  read-write
>       STATUS      current
>       DESCRIPTION
>          "The value of this object identifies the row in the 
> ADSL2 Line
>           Configuration Templates Table, (Adsl2ConfTemplatesTable),
>           which applies for this ADSL2 line.
>    s/(Adsl2ConfTemplatesTable)/(Adsl2ConfTemplateTable)/
>    i.s Template is singular.
> 
> - adsl2LineStatusLastStateDs
>   I do not understand the "counting" in
>      DESCRIPTION
>         "The last successful transmitted initialization state in
>          the downstream direction in the last full initialization
>          performed on the line.  States are per the specific ADSL type
>          and are counted from 0 (if G.994.1 is used) or 1 (if G.994.1
>          is not used) up to Showtime."
>   I guess you mean that the values for this stats are numbered from 0
>   up to showtime. I also wonder if it is wise to have it here as
>   opposed to in the TEXTUAL CONVENTION, so you do not have to repeat
>   it every time you use the TC as a SYNTAX. Besides, new values 
>   may get added to the TEXTUAL-CONVENTION in the future, so you would
>   then possibly also have to update thus text.
> 
> - Same for adsl2LineStatusLastStateUs
>   there may be others?
> 
> - adsl2LineStatusAtur
>       SYNTAX      Adsl2LineStatus
>       MAX-ACCESS  read-only
>       STATUS      current
>       DESCRIPTION
>          "Indicates current state (existing failures) of the ATU-R.
>           This is a bit-map of possible conditions.  The various bit
>           positions are: noFailure(0), lossOfFraming(1),
>           lossOfSignal(2), lossOfPower(3),
>           initFailure(4) - never active on ATU-R"
>   In the Adsl2LineSTatus, the value zero is labeled as 
> noDefect instead
>   of noFailure.
>   Again, repeating the values here in the OBJECT seems redundant, plus
>   is is prone to error/inconsistencies.
> 
> - same for 

>   there may be others
> 
> - WHen I read this:
>    adsl2LineStatusLnAttenDs  OBJECT-TYPE
>       SYNTAX      Unsigned32
>       UNITS       "0.1 dB"
>       MAX-ACCESS  read-only
>       STATUS      current
>       DESCRIPTION
>          "The measured difference in the total power 
> transmitted by the
> 
>           ATU-C and the total power received by the ATU-R 
> over all sub-
>           carriers during diagnostics mode and initialization.  It
>           ranges from 0 to 1270 units of 0.1 dB (Physical values
>           are 0 to 127 dB).  A value of all 1's indicates the line
>           attenuation is out of range to be represented."
>   Then I would have used a syntax as follows:
>       SYNTAX      Unsigned32 (0..1270 | 4294967295)
>   And U would use (in DESCRIPTION clasue):
>        A value of 429494967295 indicates the line attenuation is out
>        of range to be represented
>   That way you make it more readable to non-binary folk.
>   But more important, the SYNTAX clause now explicitly and exactly 
>   defiones the possible values.
> 
> - same for adsl2LineStatusLnAttenUs
>   there may be others, in fact next object is also the same
>   Looks like a candidate/potential TC to me as well.
> 
> - WHen I See this:
>    adsl2LineStatusSnrMarginDs  OBJECT-TYPE
>       SYNTAX      Integer32
>       UNITS       "0.1 dB"
>       MAX-ACCESS  read-only
>       STATUS      current
>       DESCRIPTION
>          "Downstream SNR Margin is the maximum increase in dB of the
>           noise power received at the ATU-R, such that the BER
>           requirements are met for all downstream bearer channels.  It
>           ranges from -640 to 630 units of 0.1 dB (Physical values are
>           -64 to 63 dB).  A value of all 1's indicates the line
>           attenuation is out of range to be represented."
>   Then I wonder about the "value of all 1's", because I think that is
>   minus 1, is it not, and that means it is in the range of 
> -640 to 630.
>   So I am confused.
>   But anway, I would have done the SYNTAX aka
>       SYNTAX    Integer32 (-640..630 | nnn)
>   you must choose a vlaue for nnn (ro replace your "all 1's" I think
> 
> - In fact you have many "0.1dB" types (some as Integer32, 
> some as Unsigned32).
>   Maybe a single TC can be created for Adsl2TenthOfAdB with a 
> syntax of
>   Interger32, and then every time you use it, you specify the 
> range that
>   is valid for that specific object.
> 
> -    adsl2ChStatusPtmStatus  OBJECT-TYPE
>       SYNTAX      Adsl2ChPtmStatus
>       MAX-ACCESS  read-only
>       STATUS      current
>       DESCRIPTION
>          "Indicates current state (existing failures) of the ADSL
>           channel in case its Data Path is PTM.  This is a bit-map of
>           possible conditions.  The various bit positions are:
> 
>              noFailure(0),
>              outOfSync (1).
>          In case the channel is not of PTM Data Path the object is set
>          to '0'."
>   The TC however speaks about noDefect instead of noFailure.
>   Pls check all labels used in the adsl2MIB with the labels 
> actually defined
>   in the TEXTUAL-CONVENTIONs.
> 
> - adsl2SCStatusSnr
>   is an OCTET STRING and then in DESCRIPTION clause you speak 
> of bytes.
>   I know that some ADs have concerns that a byet is not always known
>   to be an octet. So maybe (if you do a new rev any way) then 
> use the word
>   octets instead of bytes. Maybe this occures multiple times.
> 
> - I am completely confused by the following:
>     adsl2SCStatusRowStatus  OBJECT-TYPE
>         SYNTAX      RowStatus
>         MAX-ACCESS  read-write
>         DESCRIPTION
>         "Row Status.  The SNMP agent should create a row in this
>          table for storing the results of a DELT performed on the
>          associated line, if such a row does not already exist.
>          The SNMP agent may have limited resources; therefore, if
>          multiple rows co-exist in the table, it may fail to add
>          new rows to the table or allocate memory resources for a new
>          DELT process.  If that occurs, the SNMP agent responds with
>          either the value 'tableFull' or the value
>          'noResources' (for adsl2LineCmndConfLdsfFailReason
>          object in adsl2LineTable)
>          The management system (the operator) may delete rows 
> according
>          to any scheme.  E.g., after retrieving the results."
>         ::= { adsl2SCStatusEntry 19 }
>   First, I don't think I have ever seen a read-write as MAX-ACCESS
>   for RowStatus. I think it is always read-create. In fact 
> you speak about
>   row-creation, but only by the agent ??
>   The labels of 'tableFull', 'noResources' and such do not 
> exist to RowStatus
>   (See RFC2579).  I see that these errors are supposed to be 
> returned in 
>   another object in another table... but I'd like to know 
> what happens when
>   an SNMP manager gets back when he tries to do a SET for this object
>   to say createAndWait or createAndGo.
>   So I am not sure what you are doing here.
>   Since this is a writable object, does a row persist across reboots?
> 
> -    adsl2LInvG994VendorId  OBJECT-TYPE
>       SYNTAX      OCTET STRING  (SIZE(0..8))
>       MAX-ACCESS  read-only
>       STATUS      current
>       DESCRIPTION
>          "The ATU G.994.1 Vendor ID as inserted in the G.994.1 CL/CLR
>           message.  It consists of 8 binary octets, including 
> a country
>           code followed by a (regionally allocated) provider code, as
>           defined in Recommendation T.35."
>   If I understand the description clause correctly (and 
> assuming it is 
>   complete), then I would expext
>       SYNTAX      OCTET STRING  (SIZE(8))
>   Otherwsie, what does a zero length octet string mean?
>   Same for adsl2LInvSystemVendorId
> 
> - adsl2LInvVersionNumber
>   at least talks about "up to 16 octets".
>   but what does a zerp lenmgth string mean?
> 

So what does a zero length OCTET STRING mean?
I'd like to see that explained.

> - adsl2LInvSelfTestResult
>   talks about "coded as a 32-bit integer" yet it is an OCTET STRING ??
>   and it seems the octets represent separate values. So in my view
>   not coded as a 32-bit integer?
> 
> -   adsl2LConfProfRaModeUs  OBJECT-TYPE
>       SYNTAX      Adsl2RaMode
>       MAX-ACCESS  read-create
>       STATUS      current
>       DESCRIPTION
>          "The mode of operation of a rate-adaptive ATU-R in 
> the transmit
>           direction.  The parameter can take three values:
>              manual (1),
>              raInit (2),
>              dynamicRa (3)."
>    Not in sync with the TC (capital I in raInit)
> 
> - adsl2ChConfProfMinResDataRateDs and adsl2ChConfProfMinResDataRateUs
>   InDESCRIPTION clauses:
>   s/DynamicRa/dynamicRa" to be consistent.
> 
> - adsl2ChConfProfMinProtectionUs
>   Seems to be quite out of sync with the TC that is used as SYNTAX

It currently has:
             noProtection (1),
             halfSymbol (2),
             singleSymbol (3),
             twoSymbols (4),
             fourSymbols (5),
             eightSymbols (6),
             sixteenSymbols (7)."

That is still out of sync with what I see on page 10 and 
on pages 33/34

Please get that in sync!

> 
> -    adsl2PMLineCurrTable  OBJECT-TYPE
>       SYNTAX      SEQUENCE  OF  Adsl2PMLineCurrEntry
>       MAX-ACCESS  not-accessible
>       STATUS      current
>       DESCRIPTION
>          "The table adsl2PMLineCurrTable contains current Performance
>           Monitoring results of ADSL2 line.  The objects in this table
>           are NOT persistent."
>   Why is the NOT capitalized? NOT by itself is not rfc2119 
> terminology?
>   Did you mean "MUST NOT be" or "MAY NOT be" ?
>   AS you probably know, it is best to be clear what to expect. I.e.
>   if you mean "MUST NOT be", that is good.
>   If you mean "MAY NOT be", then a manager still does not know what to
>   expect. In that cas I think you mean "the persistency behaviour is
>   agent implementation dependent". Not so good fof a mgmt 
> station though.
>   Now,.. this is a read-only table, so in fact you then do not have
>   to discuss persistency per se. The discussion of 
> persistency behaviour
>   is specifically important for read-write or read-create 
> tables/objects,
>   because a management app needs to know if a SET that succeed will
>   survive reboots or not.
> 

Better now.
But for the COunterxx objects we MUST have text that explains if/when
discontinuities can occur and if so what is the object that 
indicates when the discontinuity occured.


>   Same for adsl2LineInventoryTable... maybe other tables?
> 
> - Maybe I am missing somethingm, but in:
>    Adsl2PMLineCurrEntry  ::=
>       SEQUENCE {
>          adsl2PMLCurrUnit                    Adsl2Unit,
>          adsl2PMLCurrValidIntervals          Unsigned32,
>          adsl2PMLCurrInvalidIntervals        Unsigned32,
>          adsl2PMLCurr15MTimeElapsed          HCPerfTimeElapsed,
>          adsl2PMLCurr15MFecs                 Counter32,
>          adsl2PMLCurr15MEs                   Counter32,
>          adsl2PMLCurr15MSes                  Counter32,
>          adsl2PMLCurr15MLoss                 Counter32,
>          adsl2PMLCurr15MUas                  Counter32,
>          adsl2PMLCurr1DayValidIntervals      Unsigned32,
>          adsl2PMLCurr1DayInvalidIntervals    Unsigned32,
>          adsl2PMLCurr1DayTimeElapsed         HCPerfTimeElapsed,
>          adsl2PMLCurr1DayFecs                Counter32,
>          adsl2PMLCurr1DayEs                  Counter32,
>          adsl2PMLCurr1DaySes                 Counter32,
>          adsl2PMLCurr1DayLoss                Counter32,
>          adsl2PMLCurr1DayUas                 Counter32
>       }
>   This seems to be (to me) a table of 15 minute current intervals.
>   So why are the TCs and schem from RFC3593 not used?
>   The HCPerfTimeElapsed also confuses me a bit as it makes me think
>   we talk about hig-perfromance counters, i.e. 64 bit based?
> 
>   Maybe I need more coffee first. Anyway, sending this to the 
>   authors/WG
>   so that they can comment on this point, because based on 
>   the answer, some
>   of the other tables in the MIB module would get similar 
>   comments from me.
> 
I got amn answer back in feb. Thanks

Bert

> Bert
> 
> _______________________________________________
> Adslmib mailing list
> [email protected]
> https://www1.ietf.org/mailman/listinfo/adslmib
>