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

[Donald Eastlake]

Hi Paul,

Thanks for your comments. Sorry for the delay in response.
Please see below.

On Mon, Jun 27, 2016 at 6:46 PM, Paul Kyzivat 
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 .
>
> 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 ...

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

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

> (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...

> (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 appropri

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

[Paul Kyzivat]

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

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, 

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

[Donald Eastlake]

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.

Thanks,
Donald
===
 Donald E. Eastlake 3rd   +1-508-333-2270 (cell)
 155 Beaver Street, Milford, MA 01757 USA
 d3e...@gmail.com


On Mon, Jul 4, 2016 at 6:50 PM, Paul Kyzivat  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 
>> 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 .
>>>
>>> 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