Peter Memishian writes: > 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.
I think this case is a little different. We're not talking about something that might be used on different platforms -- it's a system design issue that user-space code can now use regular socket calls rather than constructing these packets by hand. By the same token, we wouldn't place all of the source from $SRC/lib into $SRC/common, just on the hope that someone might be able to reuse it someday. But, I don't feel strongly about it. If you feel it belongs in common even though there's really only one user, and very likely will only be one user, then that's fine by me. > > 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. OK -- that's why I said it'd be nice. If you don't want to go on that tangent, that's ok by me. Using the non-standard byte order just gives a rash, no matter what the original reason. > > 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). Actually, since we have a socket per DHCP lif and state machine instance, I was suggesting making it so that the conn_t could have at most exactly one ill_t in its set, not multiple. Yes, that'd nail dhcpagent down so that we could never use the single-socket approach. > I'll think about this some more. OK. If it were possible to make sure that any time SIOCSLIFDHCPINIT is set, we also have IFF_DHCPRUNNING set, I'd be a little less concerned. The code already scans for IFF_DHCPRUNNING interfaces on start-up, so, as a work-around, we could just disable SIOCSLIFDHCPINIT at that point. > > 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. Ah, ok. > > 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. It looks like you're establishing a new grouping, even though the previous grouping was a mixed bag. To me, it suggests that the previous group has more significance than it does, or that this one entry is somehow special and stands apart. It's a nit, but I don't think I would have put this extern off by itself. -- James Carlson, Solaris Networking <james.d.carlson at sun.com> Sun Microsystems / 35 Network Drive 71.232W Vox +1 781 442 2084 MS UBUR02-212 / Burlington MA 01803-2757 42.496N Fax +1 781 442 1677
