Re: [radext] Review request for draft-ietf-softwire-map-radius

[" Yu Fu " <[e-mail]>]

Hi Alan,


We have updated the 18 version based on all your comments at the high level and in detail.


https://datatracker.ietf.org/doc/draft-ietf-softwire-map-radius/


Please find the corresponding changes as  below:


>  At a high level:

>* Please remove all ASCII art.  RFC 8044 (RADIUS data types) Section 2.1 suggests that this is no longer necessary.

>* Please use the attribute definition format suggested in RFC 8044 Section 2.1.3.  Doing so will make the document clearer

>* Most of the data types in this document fit the data types in RFC 8044.  If not, the considerations of RFC 6158 Section 3.2.3 on "Complex >Data Types" need to be addressed.

[fuyu]: We have referenced the RFC 8044 for the attribute format and cited it in the definition. 


>* Please decrease the use of "TLV"  An attribute can be referred to by name. e.g. "S46-MAP-E".  It does not need to be referred to as "S46-
>MAP-E TLV"



[fuyu]: We have decrease the use of TLV in the section 3 of the 18 version. 



 
>  And a review in detail:

>3.  Extensions of RADIUS Attributes and TLVs

>I suggest just calling this "New RADIUS Attributes".  There already exists "RADIUS extensions" documents, which do radically different things.


>      The Softwire46-Configuration Attribute MUST contain one or more of
>      the following: S46-MAP-E TLV, S46-MAP-T TLV, and/or S46-

>      Lightweight-4over6 TLV.

>  "the following... " what?  I suggest:

>      The Softwire46-Configuration Attribute MUST contain one or more of
>      the following attributes: S46-MAP-E, S46-MAP-T, and/or S46-
>      Lightweight-4over6.

>  Similar comments apply to many other places in the document.


[fuyu]: Please see that we have deleted "TLV" in the corresponding place.



>  Also, is there any reason to shorten the attribute names?  "Softwire46-MAP-E" would be clearer than "S46-MAP-E".


[fuyu]: Please see that we have updated "Softwire46" instead of "S46" in the whole document.



>  Figure 1 is useful and informative.  NIT: I would remove the repeated use of "TLV" and "sub-TLV".  Which attribute contains what is clear from >the diagram.

[fuyu]: Done


>3.1.1.1.  S46-MAP-E TLV Format

>  I would rename this section "S46-MAP-E Attribute".  The section describes the attribute, not just it's format.  If RFC 8044 is referenced, the >formats are defined there.  And don't need to be copied here.



[fuyu]: Please see that we don't copy the format here and just cite it from the RFC 8044 in the new version.



>  The section also doesn't say what the attribute is *for*.  While that description is alluded to in the rest of the document, it it not clear.  Again, >using the layout recommended by RFC 8044 would be useful.  That layout includes a portion for a description of what the attribute does.

>  Similar comments apply to many other places in the document.



[fuyu]: We have added one sentence to describe each attribute is "for"...



>Table 2: Softwire46 Sub-TLVs

>  The use of "M - Mandatory, O - Optional, N/P - Not Permitted" is unusual.  I suggest using a table format similar to RFC 2865 Section 5.44, as > is done with other RADIUS RFCs.  e.g. use "0", "0-1", "0+". etc.


 > For number assignment, the TLVs create a new number space, and can likely be assigned directly in the document.  e.g. if "Foo" contains >"Bar" and "Baz", then you can assign "1" to "Bar", and "2" to "Baz".  This is what IANA will do anyways, and it helps clarify the document, and > prevent mistakes.



[fuyu]: Thanks for the suggestions.  We have changed it into "0, 0+ ....."



>3.1.4.3.  EA-Length Sub-TLV

>  This section defines an attribute that contains a 16 bit number.  Such attributes are discouraged by RFC 6158.  This definition should be >corrected to be a 32-bit integer, or an explanation should be given as to why 16 bits is required here.

>  Similar comments apply for Sections 3.1.6.1, 3.1.6.2, and 3.1.6.3.
>  Section 3.2.  defines an attribute that contains an array of 16-bit integers.  Please follow the guidelines of RFC 6158 here.

[fuyu]：Thanks, We have corrected them into 32 bit. 



Thanks again,


Cheers


Yu