Re: [Lsr] AD review of draft-ietf-lsr-flex-algo-20

["Acee Lindem (acee)" <[e-mail]>]

Hi John, 

On 8/18/22, 1:29 PM, "John Scudder" <jgs=[e-mail]> wrote:

    Hi Acee,

    > On Aug 18, 2022, at 1:10 PM, Acee Lindem (acee) <acee=[e-mail]> wrote:
    > 
    > Speaking as Document Shepherd:
    >  
    > Hi John, 
    >  
    > Thanks much for your review and suggested text. I think these will improve the both the precision of the specification and the readability. I read though the comments and I don’t see any show stoppers although the additional operational considerations may take some thinking. Note that the main editor just went on PTO after you completed you review and it will be at least a 3 weeks before you receive a response (and maybe more).

    Yes, Peter also mentioned that to me unicast, thanks.

    [snip]

    >     The Opaque ID field is an arbitrary value used to maintain multiple
    >     OSPFv2 EIA-ASBR LSAs.  For OSPFv2 EIA-ASBR LSAs, the Opaque ID has no
    > @@ -1220,11 +1305,28 @@
    >     TLV is padded to 4-octet alignment; padding is not included in the
    >     Length field (so a 3-octet value would have a length of 3, but the
    >     total size of the TLV would be 8 octets).  Nested TLVs are also
    > -   32-bit aligned.  For example, a 1-byte value would have the Length
    > +   32-bit aligned.  For example, a 1-octet value would have the Length
    >     field set to 1, and 3 octets of padding would be added to the end of
    >     the value portion of the TLV.  The padding is composed of zeros.
    > -
    > -
    > +   
    > +jgs: I have mixed feelings about how you cut-and-paste the definition from
    > +RFC 3630 Section 2.3.2 instead of just referencing it. On one hand, the
    > +material starting with "for example" is new, provides more clarity, and
    > +the requirement for padding to be zeroes is new. On the other hand, your
    > +reference to the Length field, which makes sense in the original context
    > +of RFC 3630 §2.3.2, is confusing here -- you have a diagram above with a
    > +field called Length, but that is NOT what you're talking about here.
    > +In 3630 there's a TLV diagram that makes it clear at a glance what's 
    > +being talked about.
    > +
    > +Again I think the easiest fix is to leave the first sentence (adding
    > +"Section 2.3.2" to the reference) and remove the rest, although if it's
    > +important to specify zero-padding then leave that sentence.
    > +
    > +On the other hand if you feel the full detail is needed for clarity,
    > +then go all the way and make this its own subsection and don't just
    > +copy-paste the definition portion from 3630, copy the TLV diagram too,
    > +so the reader isn't led astray.
    > 
    > Since the included text is only a couple sentences, I think it is clearer to include it as has been done in other OSPF documents. To avoid the ambiguity that you have pointed out, we can replace “Length field” with “TLV or Sub-TLV length”. While I don’t think replication of the TLV format in a diagram is needed, it better to have the very brief text rather than require going to RFC 3630 to learn the encoding rules. One only has to cite the infamous RFC 7752 as a document that is unwieldy, in part, due to the number of external references required for the encodings.

    Any solution that makes it clear what’s being referred to is OK of course. I do think it would be a cheap and easy thing to add a section break before that paragraph in support of that goal, something like the below. 

Breaking it into multiple paragraphs is fine with me. I don't feel strongly on section break either way. 

Thanks,
Acee

    --- 10.1-para	2022-08-18 13:26:20.000000000 -0400
    +++ 10.1-para-jgs-edits	2022-08-18 13:26:07.000000000 -0400
    @@ -1,14 +1,17 @@
    +10.1.1. EIA-ASBR TLVs
    +
        The format of the TLVs within the body of the OSPFv2 EIA-ASBR LSA is
        the same as the format used by the Traffic Engineering Extensions to
        OSPFv2 [RFC3630].  The variable TLV section consists of one or more
    -   nested TLV tuples.  Nested TLVs are also referred to as sub- TLVs.
    -   The Length field defines the length of the value portion in octets
    +   nested TLV tuples.  Nested TLVs are also referred to as sub-TLVs.
    +   
    +   The TLV or Sub-TLV length field defines the length of the value portion in octets
        (thus, a TLV with no value portion would have a length of 0).  The
        TLV is padded to 4-octet alignment; padding is not included in the
        Length field (so a 3-octet value would have a length of 3, but the
        total size of the TLV would be 8 octets).  Nested TLVs are also
    -   32-bit aligned.  For example, a 1-byte value would have the Length
    +   32-bit aligned.  For example, a 1-octet value would have the TLV or Sub-TLV length
        field set to 1, and 3 octets of padding would be added to the end of
        the value portion of the TLV.  The padding is composed of zeros.

    -10.1.1.  OSPFv2 Extended Inter-Area ASBR TLV
    +10.1.1.1.  OSPFv2 Extended Inter-Area ASBR TLV

    That adds one level of subsection nesting but I don’t see that as problematic.

    $0.02,

    —John