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

To quote Bullwinkle T. Moose, "This time for sure..."
See previous messages for context, but use this for content.

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, but
they're now mostly typos or things I can write off.  Only a few are
more than can be fixed in AUTH48.


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 reference for [4].  (and what
happened to the other DOCSIS references?)  The title and code in
quotes might be enough for a knowledgeable person to find them, but
they don't mean anything to me.  The given URL doesn't refer to a
single document, and it's not clear which of tghe several available at
that URL is intended.  I suspect again that a knowledgeable person
could figure out the relationship.  So, since one of the GenART
charges is general understanding, I suggest the reference and/or web
page need some improvement in RFC prep.

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.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 thewse 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?

Typos
-----

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 [5]" => "described in RFC 2119 [5]"
	(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: "co-ordination" => "coordination"

Section 3: "co-ordinate" => "coordinate"

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

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

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

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

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

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

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

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


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

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