AD review of: draft-ietf-policy-qos-device-info-model-08.txt

OK, here is my review (finally... blush)

- Needs to be checked against ID-NITS,
  see: http://www.ietf.org/ID-nits.html 
  Also pls check against RFC2223bis
  see draft-rfc-editor-rfc2223bis-04.txt
  Things that fall under this category:
  - no citations in abstract
  - split references in normative and informative
  - expand Acronyms when used for the first time
  - use proper references in the references section
  - is the reference to cim 2.5 correct?
  - not sure if the IP address as on page 7 is allowed
    according to ID-NITs
  - I see some Msoft characters in the doc (page 12 and 64 are
    examples)
  - many referenced documents have become RFCs now. You may want
    to update those.

- I suspect that security area is too weak.
  Specifically if you tell people to use IPSEC, you have to
  explain how that is done. But... maybe you can refer to
  PCIM and PCIMe, similar to how you did it for QPIM.
  Maybe there are some extra concerns since you also derive
  a lot directly from CIM ??

- In the abstract, I think that the last piece needs to
  be removed. That is:

                      A separate draft could be written to provide
     a mapping of the data contained in this document to a form
     suitable for implementation in a directory that uses (L)DAP as
     its access protocol.  Similarly, a draft could be written to
     provide a mapping of the data in [QPIM] to a directory.
     Together, these four drafts (information models and directory
     schema mappings) would then describe how to write QoS policy
     rules that can be used to store information in directories to
     configure device QoS mechanisms.

  Cause that tells what "could be done" but that cannot be the
  "abstract" for this document, can it?

- The document speaks about "this draft" a lot. I think it should
  be changed throughout the document into "this document". So that
  once it is published we do not see "this draft" anymore.

- Page 11.
  It might help if in the figure you indicate where we find 
  CIM, PCIM, PCIMe, QPIM, QDDIM. etc

- I see (as an example on page 16/17) QoSService as a class.
  Then the doc talks about the plural QoSServices, and I am not
  sure if it always means QoSService Instances or subclasses of
  QoSService. At other places it talks about "QoS services"
  which seems better to me

- Page 17 I see:
    Note that this work is not yet completely aligned, as there are
    differences among the DiffServ Informal Management Model, the
    DiffServ MIB, the DiffServ PIB, and this draft.  Work to finish
    aligning these drafts is in progress, and will be reflected in
    the next revision of this draft.
  Does this mean you were expecting a new rev of QDDIM anyway?
  Anyway, such text probably does not belong in a doc that goes
  for RFC publication.

- AS an example: I see on page 21 
    Implementations may recognize other <type>'s in addition to
    these.  If collisions of implementation-specific <type>'s become
    a problem, it is possible that <type>'s may become an IANA-
    administered range in a future revision of this standard.
  This document itself should not claim that it is a standard. Or
  at least not in the text. If it gets approved as stds track then
  RFC-Editor will add that in the footing. I suggest to use
  "this document" instead of "this standard".
  Pls check if there are more occurences.

- The figures on pages 26 to 32 are not consistent in their use of
  class names and such. I can understand that you need to abbreviate
  becuase of space constraints, but it might be good to do so in a 
  consistent manner, and to list the abbreviations and explain which
  exact Class they represent.

- On page 37 you claim (at top) that "is shown in figure 9" but
  I think it is actually figure 10.
  On page 38 it should be figure 11 instead of 10 (or so I think)

- I am a bit surprised to see how the descriptions of Properties is
  done. In the PCIM (RFC3060) it was done pretty formal, for example:

      NAME             CN
      DESCRIPTION      A user-friendly name of a policy-related object.
      SYNTAX           string

  Another one:

      NAME             Mandatory
      DESCRIPTION      A flag indicating that the evaluation of the
                       PolicyConditions and execution of PolicyActions
                       (if the condition list evaluates to TRUE) is
                       required.
      SYNTAX           boolean
      DEFAULT VALUE    TRUE
  
  Or yet anbother one:

      NAME             SequencedActions
      DESCRIPTION      An enumeration indicating how to interpret the
                       action ordering indicated via the
                       PolicyActionInPolicyRule aggregation.
      SYNTAX           uint16
      VALUES           mandatory(1), recommended(2), dontCare(3)
      DEFAULT VALUE    dontCare(3)

  In RFC3460 I see it done in a similar way:


   NAME             PolicyDecisionStrategy
   DESCRIPTION      The evaluation method used for policies contained in
                    the PolicySet.  FirstMatching enforces the actions
                    of the first rule that evaluates to TRUE;
                    All Matching enforces the actions of all rules
                    that evaluate to TRUE.
   SYNTAX           uint16
   VALUES           1 [FirstMatching], 2 [AllMatching]
   DEFAULT VALUE    1 [FirstMatching]

  So why is that not followed in this document?

- Does Class TockenBucketMeterService not need a deltaInterval property?
  Otherwise what does the AverageRate property mean?

- Do we still want/need a class TosMarkerService and a property of ToSValue?
  Has ToS not been obsoleted?

- Class REDDropperService derives from DropperService.
  Does the ingeritedDropperType always have the value "Random" ??

- sect 4.3.21.2 does not even tell the datatype of the property

- Sect 4.3.26
  I am being told that the proper name for a flow label is FlowLabel
  and not FlowID. Not sure how serious this is.

- Section 4.3.34
  I do not see any of the properties described, do I?

- sect 4.3.37.2
  Would it not be better to use a 32bit unsigned?

- sect 4.3.37.5
  May I assume that the buffers are being shared?
  That is not so clear from the description (at least not to me).

- Sect 4.3.40 
  Explain what WRR stands for

- sect 4.3.40.1
  Here I wonder if a 16 bit unsigned would not be more than sufficient
  Its units are "thousands" ?? Thousands of what?

- sect 4.3.4.2 
  Does this property not add just extra complexity? Or is that just me
  thinking so?
  And... what happens if both WeightingFactor and Priority are equal?

- Sect 4.4.18 and 4.4.19
  Am I missing Property descriptions?

- sec 4.4.28
  Am I missing Property descriptions?

Thanks,
Bert