> > I've completed my initial testing and self-review, so it's time to start > > code review for the DHCP client sockets changes. The webrev is at: > > > > http://cr.opensolaris.org/~meem/dhcp-sock > > A couple of random thoughts, and one comment:
Thanks for your feedback Seb; my responses are below. > usr/src/cmd/cmd-inet/sbin/dhcpagent/interface.c > > * 1322: Not a comment for you, but for SIOCSLIFFLAGS in general. There > needs to be a better interface for "toggling" a single flag in an atomic > way. Otherwise, any other flag change that occurs by another thread > between SIOCGLIFFLAGS (as on line 1316) and SIOCSLIFFLAGS (as on line > 1322) get overridden. Yep, this is a longstanding design flaw in the SIOC[GS]LIFFLAGS interface. > usr/src/cmd/cmd-inet/sbin/dhcpagent/states.c > > * 704,705: Is closing and re-opening the socket necessary? Could we not > re-bind to the appropriate address using the same socket? I'm more > curious than anything, as there's nothing wrong with the code you have. I'm not aware of a way to rebind through the sockets API. We'd also need to disable IP_UNSPEC_SRC if we did a rebind. > usr/src/uts/common/inet/ip/ip.c > > * 15116: Why check for IS_SIMPLE_IPH() here? Does the DHCP protocol > mandate that IP options must not be used by the DHCP server? If not, > then the offset to the UDP header can easily be obtained even when the IP > header contains options. There's no mandate that I'm aware of, but our DHCP client has always required IP packets without options. I've replicated that requirement in the kernel since I didn't see a need to change it. -- meem
