Re: [Gen-art] Gen-art review of draft-ietf-idr-rfc3392bis-02.txt

[Elwyn Davies <[e-mail]>]

John G. Scudder wrote:
> Thanks for the review.  A few comments and replies in line below.
..and responses also.. Thanks for the fast turnaround.
>
>
> On Dec 12, 2008, at 2:24 PM, Elwyn Davies wrote:
>> I have been selected as the General Area Review Team (Gen-ART)
>> reviewer for this draft (for background on Gen-ART, please see
>> _http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html_).
>>
>> Please resolve these comments along with any other Last Call comments
>> you may receive.
>>
>> Document: draft-ietf-idr-rfc3392bis-02.txt
>> Reviewer: Elwyn Davies
>> Review Date: 12 December 2008
>> IETF LC End Date: 25/12/2008
>> IESG Telechat date: (if known) -
>> Summary:
>> The document is almost ready for the IESG.  However after diligent 
>> searching both in the RFCs and on the IANA web site, I believe that 
>> neither the original BGP RFCs 1771/4671, RFC 3392 nor this draft 
>> define a registry for the parameter types in OPEN messages.  The 
>> CAPABILITY parameter is said to be 'Type 2'.  I presume this means 
>> the value of Parameter Type in the sense of s4.2 of RFC 4671. BTW: Is 
>> there a  'Type 0' or 'Type 1'?
>>
>> There is also one minor nit and a clarification which would help.
>> Comments:
>> IANA Considerations:  There does not appear to be a registry for OPEN 
>> message (optional) parameter types!
>
> Appendix A of RFC 4271 notes that "Optional Parameter Type 1 
> (Authentication Information) has been deprecated."  There was never a 
> type 0.
OK.  Missed that.
>
> Regarding the lack of registry, that's a good observation but I don't 
> think it's within the scope of rfc3392bis to remedy, it seems like an 
> RFC 4271 issue.  Perhaps the IDR chairs and/or ADs can address this?  
> (As a practical matter I'd be surprised if there were ever another 
> OPEN optional parameter defined, but I guess it might be worth fixing 
> anyway.)
I am inclined to agree with your final point here.. and for that reason 
I wouldn't worry too much if the registry was declared in this document 
(this would not be the first time an oversight had been cleared up in 
this sort of way) but as you say, this is a matter for the Chairs/ADs.
>
>> s4: I think it would be helpful to naive readers to make it clear 
>> that the capability (sub-)TLVs all go 'inside' the overall  
>> CAPABILITY parameter TLV in the OPEN message, and the overall length 
>> in the Parameter TLV covers the whole set of capability options (and 
>> in turn the total length of all the optional parameters....  you 
>> could run out of space just counting the counters ;-) )
>>
>> s4: The encodings of the Capability Type and Length fields are not 
>> specified.
>
> Regarding these two, I think both are already fully specified in RFC 
> 4271, and my own preference is generally to err on the side of less 
> redundancy -- or on the side of terseness, to put it less charitably :-).
>
> That said, I don't want a stylistic point to impede progress.  Below 
> are two possible rewrites -- one minimal, the other more wordy.  As 
> you might imagine, I prefer the minimal one, but I'd appreciate 
> further feedback.
>
>
> Minimal change:
>
> Replace:
>
>    This is an Optional Parameter that is used by a BGP speaker to convey
>    to its BGP peer the list of capabilities supported by the speaker.
>
> with:
>
>    This is an Optional Parameter that is used by a BGP speaker to convey
>    to its BGP peer the list of capabilities supported by the speaker.
>    The encoding of BGP Optional Parameters is specified in Section 4.2
>    of [RFC4271]. The parameter type of the Capabilities Optional
>    Parameter is 2.
I am fine with just the minimal solution to the clarification point.
>
> This (minimally :-) addresses the encoding comment.  I haven't made 
> any changes to address the "go 'inside'" comment because it seems to 
> me as though it's already clear from this existing text:
I think the few extra words provide all the clues needed.

However I think you do need to say that the type and length are encoded 
as unsigned binary integers.  In fact RFC 4271 states this explicitly 
and individually for most of the numbers but it got missed out on the 
type and length fields of the OPEN optional parameter TLV (and AFAICS 
there isn't a global statement). IMO that is all that is needed extra.

The more verbose form is indeed overkill.

Regards,
Elwyn
>
>    The parameter contains one or more triples <Capability Code,
>    Capability Length, Capability Value>, where each triple is encoded as
>    shown below:
>
> and RFC 4271 makes it clear what the overall length covers.  (I would 
> think that by now, the concept of nested TLVs is workaday enough it 
> needn't be explained every time it's used.)
>
>
> More verbose change:
>
> Replace the first part of section 4 with the following:
>
>    This is an Optional Parameter that is used by a BGP speaker to convey
>    to its BGP peer the list of capabilities supported by the speaker.
>
>    The Optional Parameter encoding is specified in [RFC4271] as a
>    <Parameter Type, Parameter Length, Parameter Value> triplet.  The
>    parameter type for the Capabilities Optional Parameter is 2.  The
>    parameter length is the length of the Capabilities Optional Parameter
>    field in bytes; that is, the total of the lengths of all enclosed
>    capabilities.  The encoding of the parameter value is given below.
>
>    The Capabilities Optional Parameter contains one or more triples
>    <Capability Code, Capability Length, Capability Value>, termed
>    "capabilities". Each triple is encoded as shown below:
>
>           +------------------------------+
>           | Capability Code (1 octet)    |
>           +------------------------------+
>           | Capability Length (1 octet)  |
>           +------------------------------+
>           | Capability Value (variable)  |
>           ~                              ~
>           +------------------------------+
>
>    The use and meaning of these fields are as follows:
>
>       Capability Code:
>
>          Capability Code is a one octet field that unambiguously
>          identifies individual capabilities.
>
>       Capability Length:
>
>          Capability Length is a one octet field that contains the length
>          of the Capability Value field in octets.
>
>          (Note that this is different from the Parameter Length as
>          defined in [RFC4271], which is discussed above, and also from
>          the Optional Parameters Length, which as defined in [RFC4271]
>          gives the length of all Optional Parameters, including but not
>          limited to this one.)
>
>       Capability Value:
>
>          Capability Value is a variable length field that is interpreted
>          according to the value of the Capability Code field.
>
> (the rest of the section remains the same).  The "clarification" about 
> length to me just seems to muddy the waters but I'm open to 
> suggestions as to a better way to write it.
>
>
> Regards,
>
> --John

_______________________________________________
Gen-art mailing list
[e-mail]
https://www.ietf.org/mailman/listinfo/gen-art