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

[Paul Kyzivat <[e-mail]>]

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]
>