On Wed, 2009-09-16 at 12:30 -0700, Erik Nordmark wrote: > The webrev for usr/src is at > http://cr.opensolaris.org/~nordmark/refactor-review/
I reviewed the changes under usr/src/uts/common/inet/iptun. Please let me know if you think I should be looking at other changes and I'll take a look. Here are my comments: usr/src/uts/common/inet/iptun/iptun.c * 746-750: This explains the implementation of conn_get_ixa(). This comment should be the intro block-comment to conn_get_ixa() in ip_attr.c. * 759: This looks like it belongs in conn_get_ixa() as there is only one possible value for the ixa_cred of the ixa returned by conn_get_ixa(), and that is conn_cred. * 761-769: This also looks like something that should be in conn_get_ixa(). * 768: Won't we need to set ixa_tsl on a per-packet basis? * 786-800: It seems like there should be some utility function to set conn addresses. For example, the fact that IPv4 addresses are stored as IPv4-mapped IPv6 addresses in the conn_t seems like an implementation detail of ip that iptun shouldn't really need to care about. * 848: iptun_get_maxmtu() doesn't set the tunnel MTU, it just returns the maximum possible link MTU for a tunnel link. The call to iptun_update_mtu() at 886 does what you want anyway (and that calls iptun_get_maxmtu() to do its job). * 862: This can't happen because we call iptun_unbind() before iptun_bind() if the tunnel is already bound. I suppose you could optimize things to make the iptun_unbind() unnecessary if you want. * 870,872: We should just have an ipcl_conn_insert() that calls ipcl_conn_insert_v[4|6]() depending on conn_ipversion. tcp_conn_request() and conn_connect() both have the same little if/else dance. * 891: The convention in this file is to use "done:" as the label, as the code that goes through here is in most cases not related to bind failure. * 1311: The current process is some dladm incantation that will go away momentarily. It's probably not meaningful to save that away as conn_cpid. * 1795: There are some places in the code where ip_get_pmtu() is called with an argument of connp->conn_ixa directly without first getting a safe reference using conn_get_ixa() (in icmp_icmp_input() for example). Is that deliberate? * 1807: I know they're not used, but it would help legibility to call the first arguments using names representing what was passed in. On a related note, there seems to be no reason why the first argument couldn't simply be a "conn_t *" instead of "void *". * 1824: You don't need a default case. * 1976: What is this call to ip_attr_connect() about? * 2029: typo: there is no iptun_get_pmtu(). * 2043: In the case of IPTUN_FIXED_MTU, don't we only need update ixa_pmtu when we initially set iptun_mtu? * 2093: The ICMP error doesn't need to have the same cred as the original packet? * 2150,2200: The cred associated with the conn_t won't necessarily be the same as the cred associated with the data. The label we want in the packet is the one associated with the data, right? Labels used to be computed using dblk creds... * 2677: The first argument is always a conn_t *, so there appears to be no reason to pass in void * that is then set to a conn_t *. * 2677: The third argument is always NULL, so why have a third argument? * 2769: The first argument is always a conn_t *, so let's not use void *. * 2769: The third argument is always NULL, let's remove it. * 2781: Can we use ira_ip_hdr_len to make this faster and simple? * 2804: I believe the answer is yes, but danmcd should confirm. In the old world, I believe an ipsec_in_t M_CTL would be pre-pended even on a clear-text packet if there was global policy present. This would cause ipsec_tun_inbound() to be called on such packets. One would thus assume that IRAF_IPSEC_SECURE should be set if there was global policy present in the ip input path(?). * 2989: Don't we lose the db_credp here? * 3183,3276: We call iptun_find_headers() twice in a row for 6to4 output processing. * 3246-3259: Can this be done before calling iptun_output_common() so that iptun_output_common() becomes a tail-call? * 3297: I'm thinking that it might make sense to unconditionally include the labeling overhead in the link MTU if is_system_labeled(). This would simplify things a bit I think and would reduce the round trips necessary to make pmtu converge on labeled systems. This isn't new with your changes, but just a fleeting thought. -Seb _______________________________________________ networking-discuss mailing list [email protected]
