Re: [Gen-art] Gen-ART Last Call review of draft-ietf-trill-ia-appsubtlv

[Paul Kyzivat <[e-mail]>]

On 7/4/16 11:35 PM, Donald Eastlake wrote:
> Hi Paul,
>
> I believe we are generally in agreement.
>
> On the table in the IANA Considerations, I have read
> https://tools.ietf.org/html/draft-leiba-cotton-iana-5226bis-15#section-1.1
> and I can understand why you commented as you did. But, since all the
> table entries were created by IANA actions, I still think the draft is
> OK in having that table in the IANA Considerations Section. This is
> not a case of including some technical specification in with the IANA
> Considerations.  It's still all code points.

OK. It is not a big deal.

	Thanks,
	Paul

> Thanks,
> Donald
> ===============================
>  Donald E. Eastlake 3rd   [phone] (cell)
>  155 Beaver Street, Milford, MA 01757 USA
>  [e-mail]
>
>
> On Mon, Jul 4, 2016 at 6:50 PM, Paul Kyzivat <[e-mail]> wrote:
>> Donald,
>>
>> On 7/4/16 5:26 PM, Donald Eastlake wrote:
>>>
>>> Hi Paul,
>>>
>>> Thanks for your comments. Sorry for the delay in response.
>>> Please see below.
>>
>>
>> No problem. I was just concerned that my review hadn't been received.
>>
>>
>>> On Mon, Jun 27, 2016 at 6:46 PM, Paul Kyzivat <[e-mail]>
>>> wrote:
>>>>
>>>> I am the assigned Gen-ART reviewer for this draft. The General Area
>>>> Review Team (Gen-ART) reviews all IETF documents being processed by
>>>> the IESG for the IETF Chair. Please treat these comments just like
>>>> any other last call comments. For more information, please see the
>>>> FAQ at <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>>>>
>>>> Document: draft-ietf-trill-ia-appsubtlv
>>>> Reviewer: Paul Kyzivat
>>>> Review Date: 2016-06-27
>>>> IETF LC End Date: 2016-06-28
>>>> IESG Telechat date: 2016-07-07
>>>>
>>>> Summary:
>>>>
>>>> This draft is on the right track but has open issues, described in
>>>> the review.
>>>>
>>>> This is a well written document. I was generally able to follow it
>>>> even though I know nothing about the subject.
>>>>
>>>>
>>>> Issues:
>>>>
>>>> Major: 0
>>>> Minor: 7
>>>> Nits:  2
>>>>
>>>> (1) MINOR: (Section 2)
>>>>
>>>> "Addr Sets End" is described as follows:
>>>>
>>>>    o  Addr Sets End: The unsigned integer offset of the byte, within
>>>>       the IA APPsub-TLV value part, of the last byte of the last
>>>>       Address Set. This will be the byte just before the first
>>>>       sub-sub-TLV if any sub-sub-TLVs are present ...
>>>>
>>>> But the remaining text of this section, and the examples, imply that
>>>> this is really the length of the leading portion of this TLV ending
>>>> with the last Address Set. The programmer in me says these differ by
>>>> one, and that the implied definition is the reasonable one, while
>>>> the action definition, and the name used to identify it, are wrong.
>>>>
>>>> I expect it would be difficult at this point to rename this field,
>>>> but at least the definition can be rewritten to be consistent with
>>>> the intended usage.
>>>
>>>
>>> Right. How about
>>>
>>>    Addr Sets End: The unsigned integer byte number, within the IA
>>>    APPsub-TLV value part, of the last byte of the last Address Set,
>>>    where the first byte is numbered 1. This will be the number of the
>>>    byte just before ...
>>
>>
>> OK. If you count starting from one. (I don't, but it is your draft.)
>>
>>>> (2) MINOR: (Section 5.1)
>>>>
>>>> Normally I would expect this section to request IANA to assign new
>>>> values from the AFN table for OUI...RBridge Port ID. However it is
>>>> worded as "IANA has allocated". Perhaps this is because they have
>>>> already been (pre)allocated. I have no problem with that if IANA is
>>>> OK with it.
>>>
>>>
>>> Yup, it say "IANA has allocated" because they are already allocated. See
>>>
>>> http://www.iana.org/assignments/address-family-numbers/address-family-numbers.xhtml
>>
>>
>> OK.
>>
>>>> But IMO the references to IPv4...64-bit MAC are gratuitous and
>>>> inappropriate in an IANA Considerations section. If it is desired to
>>>> include a list of "useful" AFN values then that belongs in some
>>>> other portion of the document.
>>>
>>>
>>> I disagree. It's "IANA Considerations", not "IANA Allocation Actions".
>>> Someone looking for code points is likely look in the IANA
>>> Considerations section.  All the values shown are from the same IANA
>>> registry.  I can see no advantage to splitting this table between two
>>> different parts of the draft.
>>
>>
>> When I wrote this comment I had in mind the following that I recently read:
>>
>> https://tools.ietf.org/html/draft-leiba-cotton-iana-5226bis-15#section-1.1
>>
>>>> (3) MINOR: (Section 5.1)
>>>>
>>>> The "new" values here (OUI, MAC/24, MAC/40, IPv6/64) give "This
>>>> document" as their reference. But anyone consulting the IANA
>>>> registry and following it to this document would have difficulty
>>>> finding any *definition* of these things.
>>>>
>>>> Section 6 discusses some operational issues with them, but at best
>>>> implies a definition. (RFC7042 might be considered a definition of
>>>> OUI, though it doesn't seem to say how big it would be.)
>>>>
>>>> I think what is needed are explicit definitions of all of these,
>>>> including their widths. (In order to provide enough bits to complete
>>>> a MAC/24 it must be at least 24 bits wide, but that would be bigger
>>>> than needed for a MAC/40.  So I guess it must be at least 24 bits,
>>>> and when used to expand a MAC/24 or MAC/40 an appropriate number of
>>>> its high order bits are used.)
>>>>
>>>> It would be good for there to be a section, appearing in the TOC,
>>>> for each of these so that someone coming here from the IANA registry
>>>> will easily be able to find the definition.
>>>
>>>
>>> This is a good point. Better definitions of these AFN types and better
>>> references, either to within this document by explicit pointers to a
>>> section within another document or both, are good points. Probably
>>> Section 6 should be expanded and sub-sections added to it...
>>
>>
>> WFM
>>
>>
>>>> (4) MINOR: (Section 5.2)
>>>>
>>>> This section defines a new registry with Expert Review as the
>>>> procedure for approving new entries. What I don't see is any
>>>> guidance to the expert on appropriate criteria to use to judge
>>>> suitability of new entries. Without any guidance, relying on the
>>>> whim of the expert can lead to variable, and perhaps biased,
>>>> results.
>>>>
>>>> It would be good to give guidance on: what sorts of document
>>>> reference are acceptable, what information needs to be included in
>>>> the reference document, whether "special" values may be requested
>>>> (versus just assignment in order requests are received), and the
>>>> sorts of properties that are appropriate.
>>>
>>>
>>> OK. Some guidance can be added.
>>>
>>>> (5) MINOR: (Section 6)
>>>>
>>>> This section talks about the handling of OUI and IPv6/64 when they
>>>> appear in a Fixed Address sub-sub-TLV. It says nothing about their
>>>> meaning if these appear elsewhere, such as in a Template. I presume
>>>> this kind of usage is nonsense, but it would be better to explicitly
>>>> state it.
>>>
>>>
>>> OK, the draft should explain their processing wherever they occur.
>>>
>>>> (6) MINOR: (Section 6)
>>>>
>>>> The description of IPv6/64 says:
>>>>
>>>>    For this purpose, an 48-bit MAC address is expanded to 64
>>>>    bits as described in [RFC7042].
>>>>
>>>> It wasn't entirely apparent to me what part of 7042 covers that. It
>>>> would be helpful to provide the section where this aspect is
>>>> specified. (After some study I guess that it is section 2.2.1.)
>>>
>>>
>>> OK.
>>>
>>>> (7) MINOR: (Section A.2)
>>>>
>>>> I believe that the values of both 'Length' and 'Address Sets End'
>>>> are too small by 7 - presumably because they forgot to count the
>>>> fixed fields. This also applies to the "alternative" using explict
>>>> AFN encoding.
>>>
>>>
>>> Thanks for catching that there is an error here.
>>>
>>> Length should be the size everything after the 2-byte length
>>> field. That's
>>>   7  fixed fields
>>>  36  three address sets, each 12 bytes
>>>   7  sub-sub-tlv one
>>>  14  sub-sub-tlv two
>>> for a total of 64 so the value is off by 10.
>>>
>>> Address Sets End should be the above less the sub-sub-tlvs, so that
>>> would be 43 and the value shown is also off by 10.
>>
>>
>> I guess I also got it wrong.
>>
>> But it was obvious to me that the examples weren't all done the same way.
>>
>>>> (8) NIT: (Section A.2)
>>>>
>>>> Based on a very quick reading, ISTM that section 2.2.1 of 7042
>>>> suggests that the IPv6 addresses being constructed this way should
>>>> start with 0x02 rather than 0x20. But I'm far from sure I understand
>>>> this correctly.
>>>
>>>
>>> Ahhh, there is indeed an error here but it is in the bottom 64 bits,
>>> which should be a Modified EUI-64 identifier, as described in Section
>>> 2.2.1 of RFC 7042. Thus the top byte of the bottom 64 bits of the
>>> resulting IPv6 addresses should be 0x02. The top byte of the entire
>>> IPv6 128-bit address should be 0x20 as shown.
>>
>>
>> OK. Like I said, I didn't really understand the details.
>>
>>         Thanks,
>>         Paul
>>
>>
>>>> (9) NIT: (Section A.2)
>>>>
>>>> There seems to be a typo in the following:
>>>>
>>>>    The OUI would them be supplied
>>>>    by a second Fixed Address sub-sub-TLV proving the OUI.
>>>>
>>>> I think "proving" should be "providing".
>>>
>>>
>>> OK.
>>>
>>> Thanks,
>>> Donald
>>> ===============================
>>>  Donald E. Eastlake 3rd   [phone] (cell)
>>>  155 Beaver Street, Milford, MA 01757 USA
>>>  [e-mail]
>>>
>>
>