Hi all,   I have reviewed draft-ietf-radext-datatypes-06 as part of the Operational directorate's ongoing effort to review all IETF documents being processed by the IESG.  These comments were written with the intent of improving the operational aspects of the IETF drafts. Comments that are not addressed in last call may be included in AD reviews during the IESG review.  Document editors and WG chairs should treat these comments just like any other last call comments.   “RADIUS specifications have used data types for two decades without    defining them as managed entities.  During this time, RADIUS    implementations have named the data types, and have used them in    attribute definitions.  This document updates the specifications to    better follow established practice.  We do this by naming the data    types defined in RFC 6158, which have been used since at least RFC    2865.  We provide an IANA registry for the data types, and update the    RADIUS Attribute Type registry to include a "Data Type" field for    each attribute.  Finally, we recommend that authors of RADIUS    specifications use these types in preference to existing practice.    This document updates RFC 2865, 3162, 6158, and 6572.”   My overall view of the document is "Ready with Issues".   ** Technical **   * Section 1, page 1: > > RADIUS specifications have historically defined attributes in terms of > name, type value, and data type.   The term "type value" sounds confusing, particularly when one is used to terms such as "TLV" meaning "Type, Length, Value". Did you mean just "type"? Something else?     * Section 1, page 1: > There is no management of data type name or definition.   Same here. Not sure if you just meant data types, or what. But this is confusing to me.     * Section 2.1.1, Page 7: > For example, the data type "vsa" will contain a data field called > "VSA-Data".   Not sure what this means. Is the data type "vsa" supposed to contain multiple "fields" (a la "C-language structures"), or what?  -- i.e., it's not clear what you mean by "data type" and "data field". Given the goal of this document, such definitions should be clearly spelled out.     * Section 2.1.2, page 7: > Attributes can usually be completely described via the Attribute Type > code, name, and data type.   The use of "type" here is confusing. Could "type" be omitted from this sentence? Besides, it seems it's the first time you refer to the "code". Is the reader expected to be familiar with what you're referring to by "code", here?     * Section 2.1.3, page 8: > The Attribute Type code, given in the "dotted number" notation from > [RFC6929].   How about s/code/number/ or just remove the term "code"?     * Section 2.1.4, page 9: > > Any new data type MUST have unique name in the RADIUS Data Type > registry.  The number of the data type will be assigned by IANA.   FWIW, here you use "number" instead of "code" (good!) -- but this should be done consistently.     * Section 2.2, page 10: > Instead, specifications which define new meaning for "reserved" > fields SHOULD describe how older implementations process those fields.   What's the point of specifying how an older implementation would behave? i.e., if this spec is going to apply to older implementations, such implementations are, by definition, no longer "older" (if they end up employing with this spec)     * Section 3.11, page 21: > If the address is all zeros (i.e. "0.0.0.0", then the Prefix-Length > MUST be set to 32.   Would you clarify why?     * Section 3.15, page 25: > > Ext-Data > > The contents of this field MUST be a valid data type as defined in the > RADIUS Data Type registry.  The Ext-Data field MUST NOT contain any of > the following data types: "concat", "vsa", "extended", > "long-extended", or "evs".   How can you tell what's the data type stored in the Ext-Data field?       * Section 3.16, page 26: > The More field is one (1) bit in length, and indicates whether or not > the current attribute contains "more" than 251 octets of data.  The > More field MUST be clear (0) if the Length field has value less than > 255.  The More field MAY be set (1) if the Length field has value of > 255. > > If the More field is set (1), it indicates that the Ext-Data field has > been fragmented across multiple RADIUS attributes. When the More field > is set (1), the attribute MUST have a Length field of value 255; there > MUST be an attribute following this one; and the next attribute MUST > have both the same Type and Extended Type.   There seems to be conflicting RFC2119 for Length == 255 && M=1 -- one is a MAY, the other one is a MUST.             ** Editorial **   * Abstract: > updates the specifications to better follow established practice.  We > do this by naming the data types defined in RFC 6158, which have been > used since at least RFC 2865.   Please change to "since at least the publication of RFC2865" or ...     * Section 1.1, page 4: > > A number of data type names and definitions are given in [RFC2865] > Section 5, at the bottom of page 25.   Please rephrase as "A number of data type names and definitions are given in Section 5 of [RFC2865], at the bottom of page 25"   * Section 2.2, page 9: > It is RECOMMENDED that such attributes be treated as "invalid > attributes", as defined in [RFC6929] Section 2.8.   Throughout the document, please replace instances like this with something like "...Section x.x of [RFCxxxx]", instead.     * Section 3.5, page 15: > s, and TLVs cannot be used, the "string" data type MUST be used. > This requirement include encapsulation of data structure   s/include/includes/     * Section 3.11, page 20: > > 0                   1                   2                   3 0 1 2 3 > 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | > Reserved   | Prefix-Length |  Prefix ... > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ... > Prefix                 | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+   There seems to be an alignment problem with the Prefix-Length field?       * Section 3.16, page 26: > > Extended-Type > > This field is identical to the Extended-Type field defined above in > Section 2.13.   There's no such Section.     * Section 4.1 and Section 4.2:   Shouldn't these two sections be part of the "IANA Considerations" section?     Regards, Will