Review of draft-ietf-nsis-qspec-18.txt

Hi all,

Please find my review of draft-ietf-nsis-qspec-18.txt.

There are some minor technical issues to fix. In general the document seems
to be ready to go for AD review,  once the editorials and technical issues
have been fixed.

Please address the issues and submit an updated version of the draft if d
one so. I will go ahead with the updated draft for the protocol write up
(i.e., the step before sending to Magnus and the IESG).

  Martin

****
General
****
I found the first part of the document (i.e., until section 5) quite
confusing and after section five quite clear. This does not mean that there
is an issue, as typically Internet drafts have the need to sketch tech
things in the beginning without having the reader already at the technical
level. I assume that the text is fine right now, as the WG has not pushed to
change it.



****
Editorial:
****

- the idnits tool has found some issues (see below)
- Add line in memo heading with: Intended Status: Informational
-  Section 1: Move to the very end of memo. It it is good to acknowledge
other people, but it is fair enough to do it at the very end, as technical
protocol stuff comes first.
- section 2: Adding a line saying that QSPEC readers should (or better
must?) read the QoS NSLP in advance would be advantageous for the whole
document. 
- section 2, page 5, para starting with "Interoperability between QoS NSIS
entities (QNEs)..." says that parameters can be flagged or not flagged. I
suggest to put the terms "flagged"/"non flagged" in brackets and write
mandatory and optional parameters. This makes it easier for first time
readers. 
- - section 2, page 5, para starting with " A local QSPEC can be defined in
a local domain with the initiator QSPEC encapsulated, ...". This paragraph
is brief but describes a very powerful mechanism of the QSPEC. I would
suggest to remove the hint the TMOD parameter and just write that the QSPEC
allows local QPSECs that are domain specific. This also helps first time
readers to understand a basic feature of  QPSEC but not the full
implementation of it.
- section 4.1, 2nd para, text in para starting with " New QoS NSLP message
processing rules can only be defined in Standard...". Is it required to say
this at this part of the document at all? I remember this again stated in
the document somewhere later on at an appropriate place.
- section 4.2 two paras below figure 1. Suddenly there is text about RESERVE
and QUERY, but no hint to the QoS NSLP at all. Adding a reference to the QoS
NSLP at this occasion would be excellent. Otherwise it  is hard to see where
those terms are coming from
- section 4.3.1, para starting with " When the TMOD parameter in included in
QoS Available, it..." should read " When the TMOD parameter are included in
QoS Available, it..."
- section 4.3.1. This section nevers talks about tmod-2! tmod-2 is
mentioned only  in sections 6.2.1 and 6.2.2.
- section 5, 1st and 2nd para suddenly use the E-/N-flag without every
explaining their meaning.
- section 5.1, again the support of local QSPECs is quite powerful but never
really good explained with a figure. For instance, the first para of section
5.1 could refer to a simple figure showing a network configuration for a
single flow, but with indications where which QSPEC is used. This instantly
makes the whole concept better undestandable.
- section 5.2.5. formatting error
- section 5.3.1 and related. You use tables to define cases and also refer
to them later in the object definition. Thus it would be good to label those
tables.
- section 5.3.5 has one formatting issue and one spelling error (s/described
In [QoS-SIG]/ described in [QoS-SIG])
- section 5.4. remove reference to NSIS extensibility document, as we do not
have it ready by now.
- section 6.1. I got confused by all the different header definition. I
would propose to split 6.1. in subsections, i.e., one for each header
definition. This improves readability.
- section 6.2.1: Why is there this line    <TMOD-1> = <r> <b> <p> <m>
[RFC2210, RFC2215]? It is now required for the object definition.
- section 8, text says " Object Types (12 bits):" but should read "QSPEC
Object Types (12 bits):".
- appendix b: add a note in the first line that this appendix should be
removed by the RFC editor before publication.



***
Technical
***
- section 5.3.1, para starting with " The QSPEC parameter IDs and values
included in the QoS Reserved...". This paragraph talks about how to set the
parameters in the RESPONSE message for case 2. It is basically fine, but the
second sentence is for me not understandable (" For those QSPEC parameters
that were also included in the QoS Available object in the RESERVE message,
their value is copied into the QoS Desired object.")
The paragraph is talking about responses, but in RESPONSE there is no QoS
desired object anymore (at least according to the table above). I probably
do misunderstand something here?
- section 6.1 qspec header element definition. The length definition in the
qspec header definition contradicts the length definition at the beginning
of section 6.1 One says "total length of qspec" the other "length is always
exept header"
- section 6.2.2., see also my comment above about tmod-2. Even after reading
section 6.2.2. I do not have any clue why there two tmod parameters.
- section 6.2.7. The slack term is 32 bits as integer. However the slack
term must only be non-negative, i.e., it is actually an unsigned int. Why is
is not specified as unsigned int? However, the current value range
definition is broken, as an integer of 32 bits cannot have a maximum range
to  2**32-1 (there are only 31 bits left...). This also holds true for other
occasions where integer values are used (e.g. section 6.2.3!
- section 6.2.11. para starting with " When excess treatment is set to
'shape', ". What is the error handling if excess treatment is set to 'shape'
and no TMOD parameter is given?
- section 7. This section is extremely short. This might a non-issue, but I
personally had the feeling the security discussion is too brief. This might
be OK, but the document authors and the WG should be prepared for tough
questions regarding this when going to the IESG. What about this document
defining just the basics, but other documents defining the real behaviour?
Is it assumed that this does not raise any new security issue?
- section 8 says in several places " A specification is required to
depreciate, delete, or modify QSPEC versions." What happens to deleted IANA
values? Is IANA allowed to re-assign those or should they be treated as
deprecated values, i.e., they cannot be used anymore, but the value is
blocked for ever?

****
idnits 2.06.01 

tmp/draft-ietf-nsis-qspec-18.txt:
tmp/draft-ietf-nsis-qspec-18.txt(2867): Found possible IPv4 address
'7.2.9.4' in position 6; this doesn't match RFC3330's suggested 192.0.2.0/24
address range.

  Checking boilerplate required by RFC 3978 and 3979, updated by RFC 4748:
  
----------------------------------------------------------------------------

     No issues found here.

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

  == No 'Intended status' indicated for this document; assuming Proposed
     Standard

  == The page length should not exceed 58 lines per page, but there was 3
     longer pages, the longest (page 6) being 59 lines

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


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

  == There are 1 instance of lines with non-RFC3330-compliant IPv4 addresses
     in the document.  If these are example addresses, they should be
changed.


  Miscellaneous warnings:
  
----------------------------------------------------------------------------

  == The copyright year in the IETF Trust Copyright Line does not match the
     current year


  Checking references for intended status: Proposed Standard
  
----------------------------------------------------------------------------

     (See RFC 3967 for information about using normative references to
     lower-maturity documents in RFCs)

  == Missing Reference: 'S' is mentioned on line 1730, but not defined
     '|  Slack Term [S]  (32-bit integer)                             |...'

  -- Possible downref: Undefined Non-RFC (?) reference : ref. 'S'

  == Unused Reference: 'RFC4506' is defined on line 2363, but no explicit
     reference was found in the text
     '[RFC4506] Eisler, M., "XDR: External Data Representation
Standard,"...'

  == Unused Reference: 'IEEE754' is defined on line 2375, but no explicit
     reference was found in the text
     '[IEEE754] Institute of Electrical and Electronics Engineers, "IEEE
S...'

  == Unused Reference: 'NETWORK-BYTE-ORDER' is defined on line 2381, but no
     explicit reference was found in the text
     '[NETWORK-BYTE-ORDER] Wikipedia, "Endianness,"
http://en.wikipedia.or...'

  -- Possible downref: Non-RFC (?) normative reference: ref. '3GPP-1'

  -- Possible downref: Non-RFC (?) normative reference: ref. '3GPP-2'

  -- Possible downref: Non-RFC (?) normative reference: ref. '3GPP-3'

  -- Possible downref: Non-RFC (?) normative reference: ref. 'GIST'

  -- Possible downref: Non-RFC (?) normative reference: ref. 'QoS-SIG'


     Summary: 0 errors (**), 9 warnings (==), 6 comments (--).
----------------------------------------------------------------------------
----


[email protected]

NEC Laboratories Europe - Network Research Division

NEC Europe Limited | Registered Office: NEC House, 1 Victoria Road, London
W3 6BL | Registered in England 2832014