Draft:  draft-munkata-iptel-isub-type-01.txt
Reviewer: Harald Alvestrand [harald@alvestrand.no]
Review Date: Tuesday 5/30/2006 8:13 AM CST
IETF LC Date: 6/6/2006

Summary: This draft is on the right track but has open issues, described 
in the review.

This seems like a fine document initially - it repairs an 
oversight/underspecification in RFC 3966.

But I have some issues with some of the details.
In particular:

- Is the current version of ISUB, using the RFC 3966 specification, 
widely deployed? If so, the recommendation in 6.1 that a gateway SHOULD 
set "isub-type" to "nsap-ia5" might be harmful; it might be better to 
omit the "isub-type" parameter" if that is interoperable with all 
deployed implementations.

- The "nsap-ia5" and "nsap-bcd" subtypes specify (in 6.1) that the 
encoded value should start with the 5th octet of the subaddress 
information element. But the only illustration in the document is the 
one in Appendix A, which shows the DSP starting at the 3rd octet. If you 
refer to an ISDN construct that has 2 more bytes before the NSAP starts, 
it would improve the clarity of the document if those 2 bytes were 
included in Appendix A.

- The "nsap" version of the ISDN subaddress seem underspecified. In 
particular, if one encodes as specified, I think the original value of 
the AFI will be lost - if I understand this correctly, encoding should 
start at the 3rd byte, not the 4th.

- The "user specific" subaddress (6.1) is similarly unspecified. "MAY 
set any value as a token" is not an interoperable specification. If you 
want to specify that this is not specified here, simply say that:

"If the ISDN subaddress is not an NSAP address, this document does not 
specify the behaviour."

You also need the symmetric case in 6 (before 6.1) to what is mentioned 
in 6.2:

      Entity MAY define other type of "isub-type" as a token in which
      case, the semantics and format of the "isub" parameter are beyond
      the scope of this document.

Note: This form of allowed extension easily generates non-interoperable systems. The classical ways of dealing with this are:
1) to specify that no other values are allowed
2) to establish an IANA registry with registration rules. 
This may not be necessary here. But the issue may want to be called out.

 - A number of uses of SHOULD and MAY where I think MUST would be more 
adequate.
In particular, saying that "nsap" and "isub-type" SHOULD be in 
hexadecimal (6.2) seems strange - it will be in hexadecimal if the rules 
in 6.1 are followed; I can't see any valid case where it would not be.



Formalities:

- isdn-subaddress is defined in RFC 3966 as:

   isdn-subaddress      = ";isub=" 1*uric

   uric                 = reserved / unreserved / pct-encoded

  You may want to mention percent-encoding as a possibility for ia5, and 
that it's not needed
   for the nsap-bcd and nsap variants.

- In appendix A, the figure is lacking an indication of size; marking 
the byte boundaries
with numbers would make it more readable:

        <----------------- NSAP Address ----------------->

        +------------------------------------------------+
        |   I D P   |                                    |
        |-----------|             D S P                  |
        | AFI | IDI |                                    |
        +------------------------------------------------+
        0     1     2    ........... octet ....         20
                          Figure 3


- In appendix A, it says that "IDI value is decimal digits". I think the 
correct phrasing is "The IDI value is normally written as a decimal 
number, 0-255". The word "null" is also ambiguous; "zero" would be better.

- A pass for checking English grammar is recommended. For instance, "the 
entity translating the message SHOULD set "nsap" to the "isub-type"" 
would more commonly be written "the entity translating the message 
SHOULD set the "isub-type" parameter to the value "nsap"". (section 6.1).