[ Most comments elided as no further discussion is needed. ] > > Comments relative to http://cr.opensolaris.org/~seb/clearview-iptun-cr2/ > > > > uts/common/net/if.h: > > > > * 684-731: Seems like all this old iftun_req-related stuff can go. > > ACCEPT: It can, and it was removed at one point in the past. An hg > merge brought it back to life. :-(
I had this same problem during Clearview IPMP. The only solution I found was to run cronjobs that looked for "old" terminology (e.g., iftun). Sad it had to come to that. > > uts/common/inet/ip/ip.c > > > > * 6523: What does `nailed' mean here? > > ACCEPT: This is a Kebe-ism meaning "processed". :-) I've clarified the > comment as: > > /* > * Enforce policy like any other conn_t. Note that > * IP-in-IP packets don't come through here, but > * through ip_iptun_input() or > * icmp_inbound_iptun_fanout(). IPsec policy for such > * packets is enforced in the iptun module. > */ Sounds good. > > * 16567-16570: This is strange vestigial logic; seems like this > > should be removed and 16597 should ASSERT(0). > > ACCEPT: Random thought: I wonder why we're okay with using "0" as the > parameter given that it takes a boolean expression... Anyway, will use > ASSERT(0) since that's what we've always used throughout. At least in userland, assert(3C) is defined in terms of zero and non-zero (which makes sense given that the bool type didn't show up until C99). > > uts/common/inet/ip/ip_if.c: > > > * 1084-1087: Dead code/comments concerning tunnel ioctls. > > ACCEPT > > > > > * 1194, 1997: Likewise. > > ACCEPT: 1194 and 1197, yes. There also appears to be a few more > instances of processing M_IOCTL. Were those all guaranteed to be the > tunnel ioctls that I removed, or can there be others? E.g, other all > other *_pending_mp_*() functions, and ipsq_xopq_mp_cleanup(). I'm > guessing that those can go as well... The ipsq_xopq_*() functions are still needed -- those clean up the ipsq_pending_mp_*() state, which is the main codepath. The ill_pending_mp_* codepaths are still used by SIOG*ARP. The comments indicate that SIOCG*ARP will only enqueue M_IOCDATAs but a cursory inspection of the code suggests it may enqueue M_IOCTLs; I think you'll need to do some testing here to find out what can be removed. > > * 18811: Not your doing, but how can we get here with a non-zero > > ill_sap, and why would we want to preserve the old ill_sap > > value? > > You're referring to the old 18811, right? Yes, sorry. > I don't know, there seems to be quite of bit of voodoo involved in > setting ill_sap (it looks like it might get set three times to the same > value during the plumbing of an IP interface!). Where do we preserve > the old ill_sap value? The code only sets ill_sap if it's zero -- so if ill_sap is already set to a value, that value is preserved. > I'd rather not mess with this logic at this point. I think we need a > "clean up IP plumbing logic" RFE where we handle all such irrational > mechanisms (this isn't the only one). OK. > > * 19676: I presume this is just our own custom, and not an > > algorithm suggested by an RFC? Incidentally, if the assumption > > is that the uniqueness of the IP address will ensure the > > uniqueness of the interface ID, it seems that failure to account > > for endianness breaks that assumption. > > It's a suggested convention described in RFC 4213: > http://tools.ietf.org/html/rfc4213#section-3.7 > > Note that the interface-ID only needs to be unique on the link, not > globally. The only thing that's important is that both sides pick > different interface-IDs. It's also important that both sides agree on > what the other's address is, but that is left to protocols such as > IPV6CP or a tunnel negotiation protocol (link tunnel broker) that end up > explicitly setting the link local addresses to negotiated values. > > In any case, I've added a reference to the RFC in the intro block > comment for this function. Sounds good. > As for your mention of endianness, where did I get this wrong? The > "physical" address is already in network byte order. That's my mistake; I didn't realize `physaddr' was already in network order. > > * 19692: Similar comments as 19676. > > > > This one isn't documented in any RFC that I remember, but it's a > convention that seems to be used by various OSs. Note that I didn't > invent this logic here, I just moved it into the media-table driven > mechanism from the now removed ipif_set_tun_llink() function. I see. > > * 19936-19939: Seems like a bugfix; is there a CR? > > ACCEPT: Ironically, it was a bug for all link types except for tunnels. > In the previous code, the ip module would intercept the M_IOCACK for the > old SIOCSTUNPARAM ioctl and change the link-local address by peeking > inside the iftun_req. It was a total hack-job. The hack is replaced by > promoting tunnel addresses to the link-layer addresses they really were. > When they change, a DLPI notification is generated by GLDv3 and the > right thing happens here. > > For other types of links, this wasn't handled correctly, and this is CR > 4289774. It looks like I need to work on this fix a tad more, though. > I need to make sure that if the token is manually set, that we don't > automatically change it from under the admin. It'd be nice to get that antique fixed along with this wad. > > uts/common/inet/ip/ip_classifier.c: > > > > * 311: 389 because? (Yes, I know it's prime -- but so are 383 > > and 397.) > > ACCEPT: Will add a comment, but I'm not sure what to say other than > "it's a good enough prime number"... Why we think it's big enough would also be useful. Given that we generally strive for hashes with an average bucket length of 1, 383 seems a bit small, unless there's some dynamic resizing somewhere. > > uts/common/inet/ip_classifier.h: > > > > * 528-537: Out of curiosity, where do these hash algorithms come > > from? Have we confirmed that they are well-balanced in common > > use? > > I don't remember how these came to be. I can provide some real-world > distribution data this week based on the workloads on the punchin > servers. Sounds good. > > uts/common/fs/dev/sdev_netops.c: > > > > * 69: zone_check_datalink() seems to be assuming system call > > context (e.g., it does copyinstr()); seems strange to call it > > from this context. > > Hmm? zone_check_datalink() was modified to be called from the kernel > (see changes to uts/common/os/zone.c). The ZONE_CHECK_DATALINK system > call does the copyin/copyout calls outside of the context of this > function now. I must've accidentally looked at libc's implementation and driven down into ZONE_CHECK_DATALINK. On second check, the code looks fine. > > * 218, 222: Can simply pass NULL for the `link' argument. > > I don't understand your comment. The dls_mgmt_get_linkinfo() fills in > the link argument with the name of the link associated with linkid. Somehow I'd missed that we actually used `link' later in the function and had concluded that we were only calling dls_mgmt_get_linkinfo() to check if the link existed. -- meem
