RE: AD review: draft-ietf-adslmib-adsl2-05.txt

Can the authors/editors at least confirm that they have
seen this and that they are working on answers/solutions?

I know it took me far too long to get the comments to you,
so it is unfair to start being impatient. 
At the other hand, I step down as AD (as announced back
in november) at the upcoming IETF, and if we can finish
this before then, that would be good.

Thanks, Bert

> -----Original Message-----
> From: [email protected] [mailto:[email protected]]On
> Behalf Of Wijnen, Bert (Bert)
> Sent: Sunday, January 29, 2006 11:26
> To: [email protected]; [email protected]
> Cc: Adslmib (E-mail); 'Ray, Robert'
> Subject: [Adslmib] AD review: draft-ietf-adslmib-adsl2-05.txt
> 
> 
> Thanks for the new revision, which addresses my earlier
> comments. I ended my previeous review with:
> 
>    I need to study this more and better before I can complete
>    my review.
> 
> Which I did and I took a very close look at many things.
> 
> Here are my remaining comments (I think there are enough
> things below that a another new revision makes sense):
> 
> 
> - W.r.t. 
>     adsl2TCMIB MODULE-IDENTITY
>        ...
>          ::= { transmission xxx 2} -- adsl2MIB 2
>     -- RFC Ed.: we suggest to put it under { transmission 230 
> } because
>     --          this is the first available number.
>   I suspect this may be confusing for IANA, so I suggest:
>     adsl2TCMIB MODULE-IDENTITY
>        ...
>          ::= { transmission xxx 2 } -- adsl2MIB 2
>     -- IANA, the xxx here must be the same as the one assigned
>     --       to the adsl2MIB below.
>     -- RFC Ed.: Pls fill in xxx once assigned by IANA.
>   That also means you need to IMPORT mib-2 instead of transmission
> 
> - W.r.t. 
>     adsl2MIB MODULE-IDENTITY
>     ...
>           ::= { transmission xxx }
>     -- RFC Ed.: we suggest to put it under { transmission 230 
> } because
>     --          this is the first available number.
>   I would change that slightly into:
>           ::= { transmission xxx } -- To be assigned by IANA
>     -- IANA, we suggest to put it under { transmission 230 } 
> because         
>     --          this is the first available number.
>     -- RFC Ed.: pls fill in xxx once IANA has made the assignment.
> 
> - 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) !!??
> 
> - 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?
> 
> -    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 adsl2LineStatusAtuc
>   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?
> 
> - 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
> 
> -    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.
> 
>   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.
> 
> Bert
> 
> _______________________________________________
> Adslmib mailing list
> [email protected]
> https://www1.ietf.org/mailman/listinfo/adslmib
>