Thanks for your feedback, Jim. I've removed items that were ACCEPTed without further comment. I've also respun the webrev.
> Peter Memishian writes: > > http://cr.opensolaris.org/~meem/dhcp-sock > > cmd/cmd-inet/sbin/dhcpagent/Makefile > > It looks like udp_sum.c is used only by stand/lib/inet now. It > could possibly be moved out of $SRC/common. (I don't care too much, > but reducing the odd things in $SRC/common seems like a worthwhile > fix.) DISCUSS. While I see your point, I don't like making things less common. To draw an analogy, I wouldn't move code out of uts/common just because a fix I made caused it to only be used by SPARC. > cmd/cmd-inet/sbin/dhcpagent/interface.c > > 1257,1275: I find the use of host-byte-order for addresses to be > pretty confusing because it's contrary to all other networking > practice, so if this could be limited or removed, that'd be nice. DISCUSS. The code follows the existing convention for bind_sock() (which is called by the code in question). I'm OK with removing it but I'd like to do it throughout, and that seems like tangential work. > 1370: the design of this doesn't look entirely safe to me. If > dhcpagent were to go down on some fatal signal, then it appears that > we'd leave the SIOCSLIFDHCPINIT flag set on the LIF in the kernel. > I realize that we set IFF_DHCPRUNNING and can leave it set in some > cases, but this looks potentially more harmful. Is there any way to > make it less harmful, such as perhaps associating the flag with a > socket (which will then automatically close on exit)? > > Or, maybe, I shouldn't worry so much. DISCUSS. This concerns me too, but I don't think a flag would work since a single conn_t can set SIOCSLIFDHCPINIT on multiple ill_t's, and multiple conn_t's could (in theory) set SIOCSLIFDHCPINIT on the same ill_t (we could declare the second case to be unsupported, but that might come back to bite us some day). I'll think about this some more. > cmd/cmd-inet/usr.sbin/in.routed/output.c > > 198: I avoid the name 'sin' for variables, as some versions of gcc > treat that as a built-in name (even when <math.h> isn't included), > and will freak out if they see it in any other context. Suggest > 'sinp' instead. ACCEPT. For what it's worth, the code already uses `sin' in several places. However, since it really is a pointer here, I've made the change. > uts/common/inet/ip/ip.c > > 20258: can xmit_ill be non-NULL here, and thus lose a reference by > this assignment? Line 20274 seems to say this can happen. REJECT. The only earlier codepath that sets xmit_ill is IP_PKTINFO, and that explicitly checks that conn_outgoing_ill is NULL before setting it. > 20281: unclear what "standard path" means here, as it looks like > we're vectoring away from the standard (fall-through) path for these > cases. > > 20303: this comment isn't true; it can't be a local ire here > anymore. ACCEPT. Both of these comments are tied to a logic error, which is that the code should've been: /* * If the destination is a broadcast, local, or loopback * address, SO_DONTROUTE and IP_NEXTHOP go through the * standard path. */ ire = ire_cache_lookup(dst, zoneid, MBLK_GETLABEL(mp), ipst); if ((ire == NULL) || (ire->ire_type & (IRE_BROADCAST | IRE_LOCAL | IRE_LOOPBACK)) == 0) { ^^^^^ I've also removed the needless check for ire == NULL after the `if' statement. > uts/common/inet/ip_if.h > > 463: nit: what's the intended grouping (based on whitespace) here? DISCUSS. I think the original idea was to group related ioctls and use a blank line to separate the groups -- but that convention has not always been obeyed over the years. -- meem
