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

["John G. Scudder" <[e-mail]>]

Thanks for the review.  A few comments and replies in line below.

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.

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

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

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:

    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