Document: draft-ietf-ipcdn-qos-mib-10.txt
Reviewer: Michael A. Patton
Date: 2 februari 2005 


Review of draft-ietf-ipcdn-qos-mib-10.txt

Summary: This draft is basically ready for publication as a Proposed
	Standard RFC, but has nits that should be fixed before
	publication.

However, I'll note that I actually have a LOT of nits below, which may
call for respinning it one more time before it goes to RFCed, but
still they're really only nits...


Disclaimer:  I know enough about MIBs to be dangerous, but not enough
to be authoritative.  My review of the document (consistent with the
GenART charter) is primarily for readability and consistency.


I had only one substantive comment:

I note that you have both "...TimeCreated" and "...TimeActive", but
isn't it the case that sysUpTime-"...TimeCreated" = "...TimeActive"?
That means that keeping both entries in every row is just extra work
and only one is needed.  Likewise in the Log table keeping all three
of "...TimeCreated", "...TimeActive", and "...TimeDeleted" is also
redundant duplication.  It's probably too late in the cycle to really
expect to fix this, but it seems like it would be an improvement to
drop all the "...TimeActive" and maybe include a comment on the simple
way to compute it.


There were a number of things that worried me:

I'm not sure about the quality of the references for [4], [6], [8],
and [12].  The title and code in quotes might be enough for a
knowledgeable person to find them, but they don't mean anything to
me.  I had no success finding any of them through the given URL (they
all are given as the same URL), either.  I suspect again that a
knowledgeable person could figure out the relationship.  So, since one
of the GenART charges is general understanding, I suggest these
references need some improvement in RFC prep.

[see "not a MIB expert" disclaimer] The top level MODULE-IDENTITY
seems to have 2 DESCRIPTIONs, shouldn't they be combined?

I notice that the masks for IP addresses default to all ones, while
the mask for dest MAC address defaults to all zeroes.  This seems strange
to me, it seems that they should default the same.  If there's some
real reason for the distinction it should be explained somewhere.  If
not it should be considered making them the same.  To my personal
taste all ones seems to make more sense.

It also seems strange that only the dest MAC can be masked, and the
source MAC can't, but I assume that's inherited from the master spec I
can't find, and therefore not something that could be addressed (no
pun intended) in this document.


Simple suggestions:
-------------------

Section 1.1: "described in STD 58, RFC 2578 [1], STD 58, RFC 2579 [2]
	and STD 58, RFC 2580 [3]." seems a bit awkward, I would have
	written it as "described in STD 58: RFC 2578 [1], RFC 2579 [2]
	and RFC 2580 [3]."

Section 1.2: the definition of SFID makes a point of saying that it's
	unsigned, but the definition of the SID doesn't say if it's
	signed or not.  Since these are just tokens for lookup, that
	probably doesn't matter, but it would be nice if these were
	more similar...

In Section 2.2.3 the second paragraph essentially contains a
	definition for "primary SF", shouldn't that be defined in the
	glossary?

The DESCRIPTIONs for docsQosPktClassUserPriLow and
docsQosPktClassUserPriHigh have the same words in the second
paragraph, but the justification differs.  It would help reinforce
that they are identical if they were justified the same.

Typos
-----

Section 1: "IP over Canle Data Network (IPCDN)"
        => "IP over Cable Data Network (IPCDN)"

Section 1.2: "connecting a subscriber's LAN the CATV RF network."
	  => "connecting a subscriber's LAN to the CATV RF network."

Section 2: "described in [7]" => "described in RFC 2119 [7]"
	(for consistency with other references)

Section 2.2.1: "an CATV" => "a CATV"

Section 2.2.2.1: "both DOCSIS 1.0, DOCSIS 1.1, and DOCSIS 2.0"
	      => "all of DOCSIS 1.0, DOCSIS 1.1, and DOCSIS 2.0"
	or maybe you meant
	      => "DOCSIS 1.0 as well as DOCSIS 1.1 and/or DOCSIS 2.0"

Section 2.2.4: "the number of packet delayed"
	=> "the number of packets delayed"

Section 2.2.11: "provides describes" => "describes"

Section 2.2.11: "mac addresses" => "MAC addresses"

Section 3: "with it Service Flow Classifier table"
	=> "with its Service Flow Classifier table"

Section 3: The second paragraph contains a line-break hyphenation, as
	well as one word that was hyphenated and then erroneously put
	back together without removing the hyphen.

In MIB definition of docsQosPktClassBitMap: "A bit of of this object"
	=> "A bit of this object"

In MIB definition of docsQosParamSetTosAndMask: "the this object"
	=> "this object"

In MIB definition of docsQosParamSetType: "reserved by by the"
	=> "reserved by the"

In MIB definition of docsQosServiceClassPolicyStatus:
	"it is reference by" => "it is referenced by"

In MIB definition of docsQosPHSTable: "table describes set of"
	=> "table describes the set of"

In MIB definition of docsQosPHSMask: "which used in"
	=> "which is used in"

In MIB definition of docsQosPHSMask: "corresponding to first byte"
	=> "corresponding to the first byte"

In MIB definition of docsQosPHSMask: "whether of not"
	=> "whether or not"


In MIB definition of docsQosCmtsMacToSrvFlowTable: "provide for"
	=> "provides for"

Section 6: "a agent" => "an agent"

In Section 7 the value to be supplied is "xx" (as it is in the MIB),
but it then says IANA should "assign a value for yy" and that the RFC
editor should "replace yy", both of those "yy"s should also be "xx".