not sure where the ordering requirements have come in. no need from my POV. feel free to drop it. /hannes
> On 3 May 2017, at 17:18, Peter Psenak <[email protected]> wrote: > > Hi Stig, > > please see inline: > >> On 27/04/17 00:08 , Stig Venaas wrote: >> Hello, >> >> I have been selected as the Routing Directorate reviewer for this >> draft. This is just an early QA review. >> >> The draft is in good shape, but I did find some minor issues and nits. >> It is fairly readable, but it could be improved in a few places. >> >> >> Minor issues: >> >> In 3.1: >> The SR-Algorithm TLV is some places called a Sub-TLV. It might be >> good to be consistent. > > fixed. > >> >> This is not clear in 3.1: >> The SR-Algorithm Sub-TLV is optional. It MAY only be advertised once >> in the Router Information Opaque LSA. >> Is this trying to say that it MUST NOT be advertised more than >> once? With the current wording this is not obviously that strict. > > yes, I changed the text to: "It MUST only be advertised once in the Router > Information Opaque LSA". Hope that is clear enough. > > >> >> I see some text regarding multiple SR-Algorithm sub-TLVs, but it also >> looks like one can have multiple algorithms in one sub-TLV. At least >> from the diagram. > > yes, multiple algorithms in the same SR-Algorithm TLV is fine. > >> But I don't see any discussion about this. Is it OK >> to add multiple? When can it be done, what does it mean? What if >> routers don't support the exact same set of algorithms? > > router advertise all algorithms it supports. Each router may support > different set. > >> >> The term "lowest flooding scope" is used a couple of places. I think I >> know what it means, but it might be good to point it out. Also, I'm >> used to seeing the term "smallest" rather than "lowest". I'm assuming >> they mean the same. > > I changed all places to "narrowest flooding scope". > >> >> In 3.2 there is this bullet point: >> The receiving router must adhere to the order in which the ranges >> are advertised when calculating a SID/label from a SID index. >> >> You probably should use MUST here. > > fixed. > >> >> Section 4: >> In section 4 there is a range for advertising a range of prefixes. >> But it looks like it contains a single prefix length and it says >> the length is the length of the prefix. While it says range size >> is the number of prefixes. I don't understand from the text what >> really prefix length and range size means and how this should be >> used. > > 10.0.1.0/24 - 10.0.100.0/24 represents a range of 100 /24 prefixes, where: > > (starting) prefix - 10.0.1.0 > length of the prefix is 24 > range size is 100 > >> >> I understand this is IPv4 only since OSPFv2, but rather than just >> saying IPv4 is 0, maybe refer to an IANA AF registry? This might >> be helpful if you want to use the same sub-TLV in OSPFv3 and >> use the same code for parsing etc. IANA has 1 for IPv4 though. > > this is equivalent to section 2.1 of RFC7684. I have updated the text to > match RFC7684. > >> >> Section 5: >> Is it intentional that the flags start in position 1 rather than >> 0? > > yes. Originally we had N-flag (Node Flag) defined at position 0, but we moved > that to the OSPFv2 Extended Prefix TLV (section 2.1 of RFC7684). Due to an > existing implementations, we did not shift all other bits after that. > >> >> I see that the NP flag should be ignored when M is set. Then I >> see this text: >> As the Mapping Server does not specify the originator of a prefix >> advertisement, it is not possible to determine PHP behavior solely >> based on the Mapping Server advertisement. However, PHP behavior may >> safely be done in following cases: >> This seems not very precise. Could you say exactly what the behavior >> should be, rather than saying "behavior may be done"? > > would changing "may safely be done" to "SHOULD be done" be sufficient? > >> >> Section 6: >> It might be good to make clear that other flag positions are >> reserved, set to 0 and ignored... Perhaps also point out that >> weight is in the range 0-255 > > fixed both. > >> >> I see this sentence: >> If the SID/Label Sub-TLV appears in the SID/Label Binding Sub-TLV >> more than once, instances other than the first will be ignored and >> >> Should it say MUST be ignored? > > changed to SHOULD as the previous text says "SHOULD only appear once". > >> >> Section 6.2 it says: >> All ERO Sub-TLVs must immediately follow the SID/Label Sub-TLV. >> All Backup ERO Sub-TLVs must immediately follow the last ERO Sub-TLV. >> >> Should these be normative MUSTs? > > Changed both to MUST. > I'm also going to clarify this with Hannes, whether that is still required, > because I do not see equivalent text in ISIS draft. > >> >> In 6.2.1: >> It would be good for all of these to specify that other flags are >> reserved. > > done. > > >> >> >> Nits: >> The intro should perhaps mention LAN adjacency and binding SIDs? > > LAN Adjacency SID is a sub-type of the Adjacency SID. I have added sentence > about binding/other SID types. > >> >> 2nd paragraph of section 2 is confusing. It sounds like >> the Opaque LSAs in 7684 were defined for SID in particular, >> but it is a generic mechanism. Perhaps SID was the >> motivation though? > > this part is a left-over from the original draft before we split to RFC7684 > and this draft. > > I replaced the whole paragraph with: > > "Extended Prefix/Link Opaque LSAs defined in <xref target="RFC7684"/> are > used for advertisements of the various SID types." > >> >> Section 6.1: >> It says: >> The ERO Metric Sub-TLV advertises the cost of an ERO path. It is >> used to compare the cost of a given source/destination path. A >> router SHOULD advertise the ERO Metric Sub-TLV in an advertised ERO >> TLV. >> >> Is the ERO TLV the ERO Sub-TLVs defined in 6.2? It would be good to >> point that out. > > ERO Metric Sub-TLV as well as all the other ERO sub-TLVs in sectin 6.2 are at > the same level - they are all sub-TLVs of the Binding TLV. I added some text > to 6.2 to make that clear. > >> >> In 8.4.2: >> Broadcast, NBMA or or hybrid >> Extra "or". > > fixed. > >> >> Section 9: >> There are no new registries and most of the TLVs are already >> allocated? It seems there are a few new ones where it should >> probably say TBD, or say something about being suggested values. >> That was done in earlier sections. I see in some places it says >> "are allocated" here, while it says "suggested" in the definition >> of the TLV. > > section 9 lists all the updates to the four different registries. Values are > explicitly mentioned in section 9 for every single code point. > >> >> Section 10: >> It says there are responses from 2 implementers, but I see 3. > > fixed > >> >> Section 11: >> Are these really all the potential security issues? > > I'm not aware of any others. Feel free to suggest more if required. > >> >> I'm on vacation the next 2 weeks, so I may not reply to any >> emails during that period. > > I will post the new version after closing on the ERO part with Hannes and > others. > > thanks, > Peter > >> >> Regards, >> Stig >> . >> > _______________________________________________ OSPF mailing list [email protected] https://www.ietf.org/mailman/listinfo/ospf
