Stig, I'm traveling this week. I'll get you a reply over this coming weekend.
Dino > On Sep 10, 2016, at 12:15 AM, Stig Venaas <[email protected]> wrote: > > Hi > > On Thu, Sep 8, 2016 at 2:02 PM, Dino Farinacci <[email protected]> wrote: >>> Document: draft-ietf-lisp-lcaf-14.txt >>> Reviewer: Stig Venaas >>> Review Date: 2016-09-07 >>> IETF LC End Date: >>> Intended Status: Experimental >> >> Thanks Stig for your comments. See responses below. As well as a new version >> -15 not submitted yet but for your quick review. Also find a >> lcaf-rfcdiff.html file for easy diff viewing. > > Thanks. I have some comments inline. > >>> Summary: >>> I have some minor concerns about this document that I think should be >>> resolved before publication. >>> >>> The draft is almost ready but there are several places where the text >>> is a bit unclear, >> >> No problem. This document has had a long history and many opinions related >> to it has come and gone but we tried to reflect everyone’s point of view. >> >>> In section 1 I find this sentence a bit misleading since [AFI] doesn't >>> talk about the formatting. >>> >>> Currently defined AFIs include IPv4 and IPv6 addresses, which are >>> formatted according to code-points assigned in [AFI] as follows: >>> >>> Is the formatting shown here for AFI 1 and 2 defined in another >>> document? It would be good to have a reference. If it is introduced >>> in this document, then it should not be in the introduction. >> >> No it is not shown in any document. All the AFI document says is that a >> 16-bit AFI value of 1 means an IP address follows. That means 4 bytes of >> address. And 16 for IPv6 when AFI is 2. >> >> As you can see the reference to the AFI document is at the end of the >> sentence. >> >>> In section 2, first paragraph it says: >>> There is an address family currently defined for IPv4 or IPv6 >>> addresses. >>> >>> It would be better to say that address families are defined for IPv4 >>> and IPv6 addresses. >> >> Changed. >> >>> In section 3 we have this paragraph: >>> >>> The Address Family AFI definitions from [AFI] only allocate code- >>> points for the AFI value itself. The length of the address or entity >>> that follows is not defined and is implied based on conventional >>> experience. Where the LISP protocol uses LISP Canonical Addresses >>> specifically, the address length definitions will be in this >>> specification and take precedent over any other specification. >>> >>> I'm not sure what conventional experience means. Please try to find a >> >> Well like I said above, the AFI values defined in the AFI document are just >> type values and there is no length defined. So “conventional wisdom” means >> that if a spec says an AFI value 1 is IPv4, we know what follows is 4 bytes. >> Ditto for IPv6, MAC addresses, etc. > > In theory there should be standards/documents specifying this for each > of the address types, and maybe could write that people should see the > respective standards or something. I don't know if this exists for all > the types though. > >>> better way to say it. Regarding the last sentence, what if a you want >>> to define new LCAF encodings in the future? Is it good to say that this >>> specification takes precedent over any other? >> >> LCAF encodings and definitions do not change the length of an AFI encoded >> address. So I am not sure what you mean. > > The last sentence says "Where the LISP protocol uses LISP Canonical > Addresses specifically, the address length definitions will be in this > specification and take precedent over any other specification.". What > if you in the future would like to write a separate specification that > defines additional LISP Canonical address formats? > >>> In section 3 we have this paragraph: >>> >>> Length: this 16-bit field is in units of bytes and covers all of the >>> LISP Canonical Address payload, starting and including the byte >>> after the Length field. So any LCAF encoded address will have a >>> minimum length of 8 bytes when the Length field is 0. The 8 bytes >>> include the AFI, Flags, Type, Reserved, and Length fields. When >>> the AFI is not next to encoded address in a control message, then >>> the encoded address will have a minimum length of 6 bytes when the >>> Length field is 0. The 6 bytes include the Flags, Type, Reserved, >>> and Length fields. >>> >>> Can you phrase this differently? First it says that any LCAF encoded >> >> No not really. It is precise up to the sentence “When the AFI is not ..”. >> >>> address has a minimum length of 8, but then it goes on to say how it >>> sometimes is only 6. I understand what you're trying to say, but there >>> may be a better way of stating it. >> >> This special case is for some LISP control-messages where the AFI is another >> place in the message but the address is somewhere else. Rather than have the >> AFI appear twice, the LCAF length needs to be different. >> >> The length field always includes the entire contiguous set of LCAF bytes >> including type, flags, length field, etc. >> >> The language is precise and accurate. Let me know how you think stating what >> I did in this comment response can be put in without writing a lot of text >> about a special case. > > The problem I see is that first you write "So any LCAF encoded address > will have a minimum length of 8 bytes when the Length field is 0." and > then you write "then the encoded address will have a minimum length of > 6 bytes when the Length field is 0." I understand what you mean, but I > feel this is a bit confusing. > > Maybe you could say something like: > > When including the AFI, an LCAF encoded address will have a minimum > length of 8 bytes when the Length field is 0. Or start by saying that > the minimal length is 6, and then say that it will then be 8 when the > AFI is included. > >>> In section 3 it says: >>> >>> Rsvd2: this 8-bit field is reserved for future use and MUST be >>> transmitted as 0 and ignored on receipt. >>> >>> Some of the LCAFs specified in this document are using it though. Hence >>> you may need to change this text, and maybe not make it reserved. >> >> I change to say the field is LCAF Type dependent and check the LCAF Type >> definition to see what bits of this field is not reserved. >> >>> The last sentence of section 3 is: >>> >>> Therefore, when a locator-set has a mix of AFI records and >>> LCAF records, all LCAF records will appear after all the AFI records. >>> >>> This is not necessarily true. Isn't it possible that one at some point >>> in the future might use other AFI records that have AFI > 16387? IANA >>> has assigned several values beyond 16387. >> >> I will change to order must be in AFI value order. >> >>> In 4.1 it says: >>> AFI = x: x can be any AFI value from [AFI]. >>> >>> Is 16387 always or sometimes allowed? Might be good to clarify that. >>> This also applies >>> to all the other LCAF types with similar language. >> >> Always. And since 16387 is in [AFI] and the sentence above says “can be any >> AFI value from [AFI]” that implies it can be an LCAF AFI. If I said >> “including the LCAF AFI, I would have to make this change in a dozen places >> and would be repetitive. > > I'll leave this to you, but one option could be to just mention it > here, and just say that it is true for other types as well. Or mention > somewhere before this in the draft that this is allowed. But you > already say "any AFI", so that should mean that this is allowed. > >>> Section 4.4: >>> >>> RTR RLOC Address: this is an encapsulation address used by an ITR or >>> PITR which resides behind a NAT device. This address is known to >>> have state in a NAT device so packets can flow from it to the LISP >>> ETR behind the NAT. There can be one or more NAT Tunnel Router >>> (NTR) [I-D.ermagan-lisp-nat-traversal] addresses supplied in these >>> set of fields. The number of NTRs encoded is determined by the >>> LCAF length field. When there are no NTRs supplied, the NTR >>> fields can be omitted and reflected by the LCAF length field or an >>> AFI of 0 can be used to indicate zero NTRs encoded. >>> >>> It is not quite clear to me if the NTR fields here are the RTR RLOC >> >>> addresses. Does no NTR fields mean that there are no RTR RLOC addresses? >>> If AFI 0 is used, would there be exactly 1 RTR RLOC address (of length >>> 0), and that would have AFI 0? >> >> Good catch. NTR was an old term and the NAT-traversal draft now uses the >> term RTR. I will fix. >> >>> Section 4.5 first paragraph: >>> >>> Multicast group information can be published in the mapping database. >>> So a lookup on an group address EID can return a replication list of >>> RLOC group addresses or RLOC unicast addresses. >>> >>> Can you have a mix of group addresses and unicast addresses? It's also >> >> Yes, it just depends what address you put following the AFI value. >> >>> not so clear from the format what source prefixes are. Are the source >> >> I will state that when an (S,G) is encoded that is what the source address >> field is used for. >> >>> prefixes the same as the unicast RLOCs mentioned above? Can you have >>> both multiple source addresses and then multiple group addresses? Can >>> they be in arbitrarty order, or all sources followed by all groups? >> >> As shown it is not a list. And if the AFI=0 precedes the Source/Subnet >> Address field it means there is no source address encoded. >> >>> It seems one will need to inspect the address itself to tell whether it >>> is a unicast or multicast address. This is probably fine. >> >> Right. >> >>> Section 4.6 >>> Add description of Rsvd3. >>> >>> Section 4.7 >>> Add description of Rsvd3 and Rsvd4. >> >> Changed. >> >>> Section 4.10 >>> In this section there are several examples of using the AFI List Type, >>> but I miss a general definition. It appears that there can be a variable >>> number of AFIs in the list. Any number > 0? It might be useful to have >> >> Yes, a variable number. > > How about adding a few lines of text to 4.10 saying that you can have > a variable number etc., explaining briefly what the general format is. > And then say that what follows are examples? > >>> a length field associated with each AFI, or is it OK that parsing fails if >>> an AFI is unknown, so that the address length is not known? >> >> Well each AFI has an implicit length per address entry and the LCAF Type >> length has the total length. So why would we need to have yet another length >> and complicate parsing even more? Please be aware (and I’m sure you are >> being a senior coder), that the more fields you add to parse, the more >> cross-checking of field semantics and lengths have to be done. This really >> complicates the code and if part of the LCAF Type is parseable and others >> are not, then you have a question about what you use and what you discard. > > I guess I agree with you. Only issue I see is if you at some point > need an implementation to be able to parse past an unkown AFI, > basically not knowing the expected address length of the AFI. > >>> Section 4.10.3 >>> Isn't it unusual to include the 0 termination? Since you know the >>> length it is not really needed. When parsing one will need to check >>> the length either way, what should one do it the string accidentally >>> is not 0-terminated? >> >> Well the AFI definition in [AFI] doesn’t say how to terminate the string. >> But the length field will imply it. However, I wrote the >> “distinguished-name” draft to specify when AFI=17 is used (not only in a >> LCAF but by itself), how to terminate the string. I could include that >> reference, but that draft is not a working group document. >> >> So please advise. > > Currently it says: > > Length value n: length in bytes AFI=17 field and the null-terminated > ASCII string (the last byte of 0 is included). > > It might make sense to 0 terminate the DN in some cases, but at least > here we know the string length from the value of n, so I think it > would be better to drop it here. And as I mentioned, you cannot rely > on the 0 for parsing, to be on the safe side you would use n anyway. > >>> Section 4.10.4 >>> I'm assuming that the recursion is more generic than this example? >> >> Yes. >> >>> It might be worth trying to explain the more generic concept and >>> format, and make sure that this is described as just an example. >> >> I think that can raise too many combinations. It will be hard to explain why >> someone will want to recurse a lot unless we have a well defined use-case. >> The fact that an LCAF has an AFI value. It means that an LCAF can be encoded >> wherever an AFI value is allowed. >> >> This section shows a practical example and not a general one that no one >> would know how to use. >> >>> Section 4.10.5 >>> This also appears to be just an example. >> >> But a useful one that is already implemented in cisco code. >> >>> Section 5.2 >>> Key Field Num: the number of fields (minus 1) the key can be broken >>> up into. The width of the fields are fixed length. So for a key >>> size of 8 bytes, with a Key Field Num of 4 allows 4 fields of 2 >>> bytes in length. Allowing for a reasonable number of 16 field >>> separators, valid values range from 0 to 15. >>> >>> It says minus 1. Doesn't that mean that a Key Field Num of 4 indicates >>> 5 fields? >> >> I fixed the description to be more clear. >> >>> Key Wildcard Fields: describes which fields in the key are not used >>> as part of the key lookup. This wildcard encoding is a bitfield. >>> Each bit is a don't-care bit for a corresponding field in the key. >>> Bit 0 (the low-order bit) in this bitfield corresponds the first >>> field, right-justified in the key, bit 1 the second field, and so >>> on. When a bit is set in the bitfield it is a don't-care bit and >>> >>> What does right-justified mean? Does it mean that the first field is the >>> "low order" one? Basically at the end of the message? And the highest >> >> Yes, I will make that more clear. >> >>> order bit corresponds to the part of the key right after the wilcards? >>> I think this need to be explained better to ensure interoperability. >>> >>> Key: the variable length key used to do a LISP Database Mapping >>> lookup. The length of the key is the value n (shown above) minus >>> 3. >>> >>> Isn't it exactly the value n, since the length is n + 3? >> >> Yes, you are right. Fixed. >> >>> Section 5.4 >>> Length value n: length in bytes of fields that follow. >>> This is a bit confusing. I think 2+n is the length in bytes that follow >>> the lenght field. While n is the number of bytes after the JSON length. >> >> The 2 is for the JSON length field, since it is a fixed field. The “n” is >> for the remaining variable length fields which include the JSON-binary-text >> and the AFI address. I’ll make the Length description reflect this. Thanks. >> >>> Section 5.5 >>> >>> It says "AFI = x" twice in the diagram. Based on this and the way it >>> is described it might seem that the two AFI values must be the same >>> value x, but they can differ, right? I this applies to several other >>> messages types as well. >> >> I’ll change x to y in the second occurence and a description for each. >> >>> Section 7 >>> It looks like the table in the IANA considerations doesn't include all >>> the types defined in this document. >> >> That was done intentionally. The ones that are experimental are not in this >> section 7 list because there is no use-case document for it yet. Maybe the >> chairs can explain this better than me. > > I think it's still useful. If someone sees the type used, they can > look it up in the registry. It also helps avoid that someone perhaps > tries to reuse one of these types in new documents. If you really want > to use one of the code points for something else in the future, a new > document could always update the registry. > > BTW, it also makes me wonder if it is worth reserving any LCAF types > for experiments. > > Regards, > Stig > >>> I'm happy to discuss my comments if needed, and also review any >>> updates to this draft. >>> >>> Stig >> >> Let me know about the response where I didn’t make any changes. >> >> Thanks again, >> Dino/Dave/Job >> >> >> >> >> >> >> >> >>
_______________________________________________ lisp mailing list [email protected] https://www.ietf.org/mailman/listinfo/lisp
