Document: draft-munkata-iptel-isub-type-02.txt
Reviewer: Harald Tveit Alvestrand
Date: June 28, 2006

Summary: This draft has mostly addressed the issues I raised.

Only one concern remains, which is a misplaced new paragraph in section 
6. See below.
>
> 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.
This has been changed to "MAY" - which is OK with me. It would have been 
even nicer if it said " or omit the "isub-type" parameter - but the 
current specification is reasonable.
>
> - 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 octet numbers have been removed, which makes the specification less 
clear, but I think the intent is obvious enough that that's OK.
>
> - 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.
Addressed by the omission of the octet number.
>
> - 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."
The paragraph at the end of section 6.1 is now somewhat misleading: it says

      If the ISDN subaddress is not an NSAP address, the entity
      translating the message SHOULD treat the message as if neither
      "isub-encoding" nor "isub" parameters existed, unless it has a
      prior knowledge of the encoding method used.

In this case "as if neither parameter existed" means that the entity should not CREATE the parameters. Better would be:

  If the ISDN subaddress is not an NSAP address, the entity translating the message SHOULD NOT
  generate any "isub-encoding" or "isub" parameters, unless it has a private agreement with the recipient
  about what to do in this case.

The sentence as currently written belongs to the initial part of section 6, before section 6.1.

>
> 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.
I don't see any change reflecting this concern. Omitting this is OK.
>
> - 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.
>
Changed to MUST OK.
>
>
> 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.
No change. OK.
>
> - 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

Added, including fixing my naive notion that IDI was one byte..... much 
better!
>
>
> - 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).
>
>
>