Seb, My comments on IP (and a few other files I was interested in reviewing) are below. I'm still working through iptun and will send my comments on it separately. In general, I found very few issues (mostly just additional opportunities for cleanup) and found your changes to be clean -- great work!
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. uts/common/inet/ip.h: * 1965, 1972, 2047: Synchronization table (starting around line 2146) needs to be updated with the rules for these new fields. * 1986: New ip_promisc_on_phys field doesn't seem to be used. * 2039: While you're here, s/New fields/Fields/ :-) uts/common/inet/ip/ip.c: * General: the code seems to go out of its way to call BUMP_MIB() on recv_ill (2375, 12892), but RFC4293 says that we should bump the MIB on the ill that owns the address. From the RFC: Local packets on the input side are counted on the interface associated with their destination address, which may not be the interface on which they were received. This requirement is caused by the possibility of losing the original interface during processing, especially re-assembly. * 2666, 2723, 2724: Why are these cases in the default part of the switch statement? * 6365-6367: Remove stale comment about matches on tunnels. * 6523: What does `nailed' mean here? * 16396-16399, 16448: Comment seems needless. * 16447: The old code only called ipif_setlinklocal() if the IPv6 address was zero, but the new code always does it. Is this an intentional behavioral change? * 16534: Rather than "Normally called as writer", it would be more precise to say "If `ipsq' is non-NULL, caller is writer on it." (Existing comment also seems a bit strange, as it's been a while since "global" state was modified here.) * 16567-16570: This is strange vestigial logic; seems like this should be removed and 16597 should ASSERT(0). * 16574-16592: Nested switch statement seems like overkill now; we could just ASSERT(iocp->ioc_cmd == DL_IOC_HDR_INFO) on 16574 and remove the inner switch. * 27283-27289: I don't understand this code. First, why does iptun require special handling that none of the other modern ULPs require? Second, this case is for an mp of M_{PC,}PROTO but iptun_input() ASSERT()s that mp is either M_DATA or M_CTL. Is this a vestige from an older iteration of the code? If this code does need to stay, then the comments (e.g., 27250-27251) need to be updated. uts/common/inet/ip/ip6.c: * Same general comment as ip.c regarding BUMP_MIB() calls. * 1063-1064: I'm confused why this needs to be in the `default' case, rather than just in the case that handles IPPROTO_ENCAP and IPPROTO_IPV6. * 1064: Seems like `first_mp' is leaked on failure. * 3085-3086: Remove stale comment about matching on tunnels. uts/common/inet/ip/ip6_if.c: * 1154, 1162: Given that these are the only calls to MEDIA_V6INTFID() and MEDIA_V6DESTINTFID() and they ignore the return values from these functions, it would seem preferable to change ip_v6intfid_func_t to not return a value and update the functions to ASSERT() their conditions. * 1185-1198: This function would be easier to read with a local `ill' variable. * 1193: What guarantees ill_phys_addr is suitably aligned? * 1297-1298: Since success is returned, seems like this should be moved down to 1303 so that ret_nce will be set to NULL. * 3054: Stale reference to atun. uts/common/inet/ip/ip_if.c: * 1074-1075: This comment could be simplified to "This is only needed for SIOCG*ARP." * 1084-1087: Dead code/comments concerning tunnel ioctls. * 1194, 1997: Likewise. * 5376: Comment could be revised. * 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? * 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. * 19686: What ensures that `physaddr' is suitably aligned for this cast? * 19692: Similar comments as 19676. * 19936-19939: Seems like a bugfix; is there a CR? * 19936-19939: Is there a reason to do this before the call to ill_set_ndmp()? Seems safer to ensure that ill_nd_lla* have been correctly reset in case the code called from ill_setdefaulttoken() or ipif_setlinklocal() needs to make use of them. uts/common/inet/ip_if.h: * 296: Remove dead ip_extract_tunreq() declaration. * 433: Remove dead ip_sioctl_tunparam() declaration. uts/common/inet/ip/ip_multi.c: * 1391, 1440: I'm not getting it: what's the required relationship between an IP interface requiring a resolver and an IP interface supporting multicast? uts/common/inet/ip/ip_ndp.c: * 184: s/legnth/length/. More importantly, this is legacy support for e.g. x.25, right? The comment makes it sound more general. Has anyone actually confirmed that x.25 uses this mechanism? uts/common/inet/ip/ip_classifier.c: * 311: 389 because? (Yes, I know it's prime -- but so are 383 and 397.) * 1061: Would seem more efficient to `goto done' here and remove 1063-1064. * 1096: Would seem simpler and more effecient to hoist the CONN_INC_REF() to here and remove 1098-1099. * 1294, 1374, 1539, 1637: What is `protocol' when we get here? I would've expected IPPROTO_ENCAP and IPPROTO_IPV6 -- if so, why aren't those new cases in the switch statement? Is it to take advantage of the existing check_exempt_conflict*() logic? * 1868, 2072: Insert blank line to match existing style in the switch statement. 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? uts/common/inet/ipsec_info.h: * 279-281: TUN_HELLO can go. uts/common/inet/ip/ip_ire.c: uts/common/inet/ip/ip_ftable.c: uts/common/inet/ip/ip_srcid.c: uts/common/inet/ip/ipmp.c: uts/common/inet/ip/sadb.c: uts/common/inet/ip_stack.h * Reviewed, no comments. 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. * 218, 222: Can simply pass NULL for the `link' argument. -- meem
