Subject: [Tsvwg] RE: Review of draft-ietf-tsvwg-tcp-mib-extension-08.txt (fwd)

From: Matt Mathis <[email-masked]>
Date: Thu, 2 Feb 2006 18:11:41 -0500 (EST)

(I believe that the message below never actually made it out to tsvwg for some
reason.)

I will be processing all of the feedback here in the next few weeks.  Most of
the corrections are already in progress, and will just go into the document
without any further discussion.

A few of the comments need additional clarification or raise open issues that
could use some input from other people.  I will bring these up on the tsvwg
list in succession so that no one piece will be too big to swallow.

I really appreciate all of the effort by Dan and Bert to help me see some of
the errors in my ways.

Thanks,
--MM--
---------- Forwarded message ----------
Date: Sun, 27 Nov 2005 15:44:02 +0100
From: "Wijnen, Bert (Bert)" <[email-masked]>
To: "Romascanu, Dan (Dan)" <[email-masked]>, [email-masked]
Cc: "Wijnen, Bert (Bert)" <[email-masked]>, [email-masked],
     [email-masked], [email-masked], [email-masked],
     "Allison Mankin (E-mail)" <[email-masked]>,
     TCP ESTATS MIB team <[email-masked]>
Subject: RE: Review of draft-ietf-tsvwg-tcp-mib-extension-08.txt

Thanks for your review Dan!

So I expect we'll see a new revision that addresses you comments as much as possible,
and probably also a summary to the WG list explaining how your concerns are addressed,
or why (if such is the case) they should not cause changes.

Let me also add some of my comments after a quick check of myself):

  !! Missing citation for Informative reference:
  P067 L039: [RFC3291] M. Daniele, B. Haberman, S. Routhier, J. Schoenwaelder, "Tex-

The reference itself can just be removed; you already have RFC4001 in the list, which obsoletes 3291.
But RFC4001 should be NORMATIVE (all docs from which you IMPORT must be normative)
And very often, when RFCs are listed in REFERENCE clauses (or should be listed as Dan points out
in his comments), they are also normative. In otehr words, one has to understand that REFERENCEd
RFC in order to proberly implement the object that includes the REFERENCE to that RFC.

   !! Missing citation for Normative reference:
   P065 L013: [RFC2574] U. Blumenthal, B. Wijnen, "User-based Security Model (USM) for

RFC2574 is obsoleted by RFC3414.

And RFC2575 is obsoleted by RFC3415

RFC2021 and RFC2856 must be normative, you IMPORT from them!

I'd like to extend on Dan's comment 18, in that I think it is BEST to add in every DESCRIPTION clause
of read-write or read-create objects what the expected persistency behaviour is.
For objects in a ROW, you can use a an object with a SYNTAX of StorageType TC, in which case
it is clear that all writable objects in the row follow the value of the StirageType object.
FOr a row, you can also describe the persistency behaviour of writable objects in the
xxxEntry DESCRIPTION clause.

I find it strange that these object have a Counter32 SYNTAX:

   tcpEStatsAppSndMax  OBJECT-TYPE
       SYNTAX          Counter32
       MAX-ACCESS      read-only
       STATUS          current
       DESCRIPTION
          "The farthest forward (right most or largest) SND.NXT value.
           Note that this will be equal to tcpEStatsAppSndNxt except
           when tcpEStatsAppSndNxt is pulled back during recovery."
       ::= { tcpEStatsAppEntry 3 }

   tcpEStatsAppRcvNxt  OBJECT-TYPE
       SYNTAX          Counter32
       MAX-ACCESS      read-only
       STATUS          current
       DESCRIPTION
          "The value of RCV.NXT from [RFC793]. The next sequence
           number expected on an incoming segment, and the left or
           lower edge of the receive window."
       ::= { tcpEStatsAppEntry 6 }

I am missing any words about discontinity timers/possibilities for COunter32, ZeroBasedCounter32 and
ZeroBasedCounter64 objects !!??

When I see:
   TcpEStatsConnectIdEntry ::= SEQUENCE {
                 tcpEStatsConnectLocalAddressType  InetAddressType,
                 tcpEStatsConnectLocalAddress      InetAddress,
                 tcpEStatsConnectLocalPort         InetPortNumber,
                 tcpEStatsConnectRemAddressType    InetAddressType,
                 tcpEStatsConnectRemAddress        InetAddress,
                 tcpEStatsConnectRemPort           InetPortNumber,
                 tcpEStatsConnectIndex             Unsigned32
                 }
I always wonder if Local and Remote Address types can be different!??
And if not, then I personally would use just one InetAddressType object.

About the InetAddressType/InetAddress objects. I see no indication (at least not in the MIB
module itself, which is where I looked) about any restrictions. So theoretocally one could
use 'dns' as the InetAddressType. In that case, you MUST specify when such a DNS name
gets resolbed (as RFC4001 explains).

W.r.t. to Dan's comment 14.
I know that we have such statements in other MIB modules (when all the SNMPv1,v2c,v3
text just refers to the fact of those SNMP versions are not allowing more than 128 subids.).
So personally I think I can live with it. I am checking with the set of MIB doctors.

Authors, pls copy Dan and myself explicitly on any responses.

Bert

-----Original Message-----
From: Romascanu, Dan (Dan) [mailto:[email-masked]]
Sent: Sunday, November 27, 2005 14:19
To: [email-masked]
Cc: Wijnen, Bert (Bert); [email-masked]; [email-masked]; [email-masked]; [email-masked]; Allison Mankin (E-mail); TCP ESTATS MIB team
Subject: Review of draft-ietf-tsvwg-tcp-mib-extension-08.txt

At the request of the O&M AD I have reviewed draft-ietf-tsvwg-tcp-mib-extension-08.txt.

As a general assessment, I appreciate the effort of the document editors, as well as the improvements in quality in the last few versions of this document. I do not believe however that the document is yet ready and I would recommend another round of editing to fix the problems described below, as well as resolve other comments that may have been received during the review period.

Here are my detailed findings:

1. idnits has problems with the lack of an IANA considerations section (which is mandatory even if no action is required from IANA) and formatting.

idnits 1.82

tmp/draft-ietf-tsvwg-tcp-mib-extension-08.txt:

  Checking nits according to http://www.ietf.org/ID-Checklist.html:

  * The document seems to lack an IANA Considerations section.

    Checking conformance with RFC 3978/3979 boilerplate...

    the boilerplate looks good.

  Checking nits according to http://www.ietf.org/ietf/1id-guidelines.txt:

  - It seems as if not all pages are separated by form feeds - found 0 form

    feeds but 70 pages

  Miscellaneous warnings:

    None.

2. you must leave the assignment of an OID under 'experimental' to IANA and use placeholders instead:
So, please include
           ::= { experimental yyy }

Editor's Note (to be removed prior to publication): the IANA is requested to assign a value for "yyy" under the 'experimental' subtree and to record the assignment in the SMI Numbers registry. When the assignment has been made, the RFC Editor is asked to replace "yyy" (here and in the MIB module) with the assigned value and to remove this note.

The IANA Consideration section must include the request to assign yyy.

3. Usage of the UNITS clause wherever relevant, especially for counter objects (e.g. 'seconds', 'octets', etc.) although not mandatory would be helpful.
4. There are numerous instances where references are described in the DESCRIPTION clause of the objects ('see RFC ...') , instead of the optional REFERENCE clause being used. See for example tcpEStatsStackSACK or tcpEStatsStackTimeStamps. I would recommend to fix this.
5. The following phrase in the DESCRIPTION clause of  tcpEStatsStackInRecovery is hard to understand, vague (what algorithms, what conditions?) and looks without value in its current form.

           tcpEStatsStackInRecovery is a required precondition for

           some algorithms on other instruments. E.g. Some algorithms

           to estimate path properties may not be valid during

                      recovery
I suggest either to clarify or to remove it.
6. In the DESCRIPTION clause of tcpEStatsStackSoftErrorReason  while clarifying the enumeration, the numerical values need to be mentioned. e.g. belowDataWindow(1)
7. Although there are a number of objects in the MIB module with a MAX-ACCESS of read-write the Security Consideration section does not include an explicit list of these objects, and the security hazards incurred by inappropriate configuration of these objects, but only the generic boilerplate text. This is insufficient.
8. It looks that the editors did not use the latest version of the recommended Security Considerations boilerplate text, or that this text changed since the last version of the I-D. The most update text can be found at http://www.ops.ietf.org/mib-security.html <http://www.ops.ietf.org/mib-security.html> .
9. smilint returns two warnings:

tcp-mib.txt:417: [5] {index-exceeds-too-large} warning: index of row `tcpEStatsConnectIdEntry' can exceed OID size limit by 399 subidentifier(s)

This problem derives from the fact that the tcpEStatsConnectIdTable has indices with a SYNTAX InetAddress and is addressed by the text in the DESCRIPTION clauses of the index objects.

tcp-mib.txt:2177: [4] {group-membership} warning: node `tcpEStatsAppCurAppWQueue' must be contained in at least one conformance group

This needs to be fixed, I believe that the problem is known.

10. The following paragraph in the Introduction section looks odd and may be removed:

   This document is automatically generated from a database of potential

   TCP instruments.  Beware that the OIDs are still likely to change

   with future versions.  The most current version can be obtained from

   http://www.web100.org/mib/ .  Please use [email-masked] to send

   comments to the entire TSV WG.

11. The following text in the Overview section seems to rather belong in the Security Considerations section:

        The tcpEStatsConnTableLatency object determines how long

        table rows are retained after connection close, to permit

        reading final connection completion statistics.

        Changing any of these controls may affect the correctness of

        other management applications accessing this MIB.  Generally

        local policy should only permit limited write access to

        these controls (e.g. only by one management station or only

        during system configuration).

12. The DESCRIPTION clause of the TcpEStatsOperation TC is confusing and needs to be re-edited. It is not clear why a new TC needs to be created for the objects using this TC, instead using the TruthValue TC

13. There are some problems with tcpEStatsListenerCurBacklog. First, the DESCRIPTION clause speaks about a Counter, although the SYNTAX of the object is Gauge32. Then I am concerned by the fact that the text mentions that 'MAY include connections in SYN-RECEIVED state'. How can two reads performed on two different entities in similar conditions lead to the same number?

14. The DESCRIPTION clause of tcpEStatsConnectLocalAddress and tcpEStatsConnectRemAddress still mentions SNMPv1, SNMPv2c and SNMPv3. The first two are Historical, SNMPv3 is the recurrent standard track version of SNMP, the text should be replaced by simply 'SNMP'.

15. I am confused about how tcpEStatsPerfPipeSize is to be implemented. What determines if RFC 3517 is in effect, and how does a management application know this? Is there a MIB object that provides this information, if not should there be one?

16. RFC 3517 must be added to Normative References

17. In the text 'The following objects instrument our receiver window'. What 'our' means?

18. Do we expect any of the read-write objects to keep their value persistent over re-boots. If this is a requirement, it should be mentioned in the DESCRIPTION clauses of each of the respective objects. If all or none, a general notice should be made either in the MIB module DESCRIPTION clause, or in section 3

19. I am unconfortable with the definition of  tcpEStatsPathECNsignals:

"The number of congestion signals delivered via all forms of explicit congestion notification including the ECE bit and failing the ECN nonce check, etc."

This looks to open-ended to me, I would prefer an enumeration or clear references to what is supposed to be counted here.

20. The following comment is problematic: The ratio of tcpEStatsPathDupAcksOut to tcpEStatsPathDupAckEpisodes is an indication of reorder or recovery distance'. As the two objects are counters, at some point in time roll-over is a possibility. Users should be warned that they need to use deltas of consecutive readings with correction upon roll-over and not the absolute values of tcpEStatsPathDupAcksOut and tcpEStatsPathDupAckEpisodes.

21. I do not like the proprietary usage of the non-allocated enumerated errors in tcpEStatsStackSoftErrorReason:  "Implementers are permitted to assign additional codes greater than 8 such that all SoftErrors in their implementation have unique codes. Management stations are to accumulate all unassigned codes as 'otherSoftErrors'". What does 'accumulated' mean, this is not a counter object? No interoperability can be ensured for two different implementations. If there is a need to extend this object there are multiple ways to do it, for example an IANA maintained object. To pass proprietary codes proprietary objects may be defined.

22. The table tcpEStatsStackTable has 50 columns, which would make a read operation of simultaneous objects in one PDU problematic. On the other side, many of the objects in this table are optional, which makes me suspect that breaking this table in multiple tables with similar indexation organized according to the conformance options would be a better solution.

Regards,

Dan

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