Subject: [Adslmib] Re: AD review: draft-ietf-adslmib-adsl2-06.txt

From: [email-masked]
Date: Sun, 19 Mar 2006 13:36:20 +0200

Hello Bert,

I wish to thank you for your good comments.
I also wish to show a hand in favor the suggested process which will allow
us gather more comments and then produce a version that addresses all of
them.

Best Regards,
Moti

__________________

Hello Bert,

      Thank you kindly for your review.

      I agree and would be pleased if the document can be sent to IETF Last
Call now and then handle your comments and any
additional ones at the end of the last call.

Best Regards,
Menachem.

             "Wijnen, Bert                                                 
             (Bert)"                                                       
             <[email-masked]                                          To 
             om>                       [email-masked],       
                                       [email-masked]          
             19/03/2006 07:44                                           cc 
                                       "Adslmib (E-mail)"                  
                                       <[email-masked]>, "'Ray, Robert'" 
                                       <[email-masked]>, "Dan          
                                       Romascanu (E-mail)"                 
                                       <[email-masked]>                
                                                                   Subject 
                                       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-masked]
> https://www1.ietf.org/mailman/listinfo/adslmib
>

_______________________________________________
Adslmib mailing list
[email-masked]
https://www1.ietf.org/mailman/listinfo/adslmib

---

From: "Romascanu, Dan \(Dan\)" <[email-masked]>
Date: Sun, 19 Mar 2006 17:27:19 +0200

> -----Original Message-----
> From: Wijnen, Bert (Bert) [mailto:[email-masked]] 
> Sent: Sunday, March 19, 2006 7:44 AM
> To: [email-masked]; [email-masked]
> Cc: Adslmib (E-mail); 'Ray, Robert'; Romascanu, Dan (Dan)
> Subject: AD review: draft-ietf-adslmib-adsl2-06.txt
> 
> Thanks for revision 6.
> 
> 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.
> 

It's fine with me. 

Dan