Peter Memishian writes:
> First, thanks to all who came to the kick-off meeting. For those unable
> to attend, everything you need is linked from the code review webrev at:
>
> http://cr.opensolaris.org/~meem/clearview-ipmp-cr/
Next batch; one more section covered. I'll be going over the
responses to the previous batch soon, and finishing up the review.
General:
Do you need to modify SNMP as well? It looks to me like that should
be using LIFC_UNDER_IPMP, because it's an administrative utility.
What else (possibly outside of ON) should use that flag?
+++ IPMP-aware userland changes
usr/src/cmd/cmd-inet/sbin/dhcpagent/agent.c
Reviewed; no comments.
usr/src/cmd/cmd-inet/sbin/dhcpagent/bound.c
360: what happens if the IPMP test addresses handed out via DHCP are
on a separate subnet and thus need a separate set of probe targets
(i.e., "routers")? One of the weird IPMP things we still do is that
we use the gateway addresses as probe targets, and this code will
limit the source of that information.
usr/src/cmd/cmd-inet/sbin/dhcpagent/interface.c
740: logic to handle IFF_DUPLICATE and user manipulation of IFF_UP
seems to be gone. (I know we talked about this in the past ... but
I'm not sure what the strategy is here.)
usr/src/cmd/cmd-inet/sbin/dhcpagent/interface.h
usr/src/cmd/cmd-inet/sbin/dhcpagent/packet.c
Reviewed; no comments.
usr/src/cmd/cmd-inet/sbin/dhcpagent/request.c
1010: nit: is the test of pif_under_ipmp needed here?
usr/src/cmd/cmd-inet/sbin/dhcpagent/states.c
958: this looks like a policy decision to me. For IPMP test
addresses acquired by DHCP, this code says that we must not use the
interface's hardware address as a client identifier. The
higher-level choice is that we're letting test addresses 'float'
among the physical interfaces at the MAC address level.
I think the comment here should say something about avoiding using
the hardware address for the client identifier (even though that
disables server-side duplicate checks) on IPMP test addresses so
that we don't need a special escape mechanism to send DHCP messages
on a "failed" interface.
usr/src/cmd/cmd-inet/sbin/ifparse/ifparse.c
185: I think this should be PARSELOG0 instead of PARSENOW. It's
only valid on logical interface 0 (the "physical" interface), right?
usr/src/cmd/cmd-inet/usr.bin/netstat/netstat.c
Reviewed; no comments.
usr/src/cmd/cmd-inet/usr.lib/in.ndpd/main.c
151: could possibly check here that this doesn't blow off the end of
the buffer that holds optp->nd_opt_lla_hdw_addr.
179: nit: stray extra space here.
237: I don't think this works. It's adding in sizeof (*ra) twice,
and I suspect it's sending out malformed packets. The code should
instead look like this:
optlen = add_opt_lla(pi, (struct nd_opt_lla *)pptr);
packetlen += optlen;
pptr += optlen;
usr/src/cmd/cmd-inet/usr.lib/in.ndpd/ndp.c
Reviewed; no comments.
usr/src/cmd/cmd-inet/usr.lib/in.ndpd/tables.c
622: nit: probably not needed.
usr/src/cmd/cmd-inet/usr.lib/in.ndpd/tables.h
usr/src/cmd/cmd-inet/usr.lib/mdnsd/mDNSUNP.c
usr/src/cmd/cmd-inet/usr.sbin/in.routed/defs.h
usr/src/cmd/cmd-inet/usr.sbin/in.routed/trace.c
usr/src/cmd/cmd-inet/usr.sbin/ping/ping.c
Reviewed; no comments.
usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_capture.c
142: does snoop support for the IPMP virtual interfaces arrive in
some separate project? (I'm not clear on how IPMP rearchitecture
and Clearview observability relate.)
usr/src/cmd/cmd-inet/usr.sbin/traceroute/traceroute.c
Reviewed; no comments.
usr/src/cmd/nscd/nscd_frontend.c
1461: what does nscd do (or need to do) to be "aware" of IPMP
interfaces? It looks like this just injects the underlying
interfaces into the sorting rules, but do we really want to do that?
Why should IPMP test addresses get preferential treatment during the
sort?
usr/src/cmd/rcm_daemon/common/ip_anon_rcm.c
Reviewed; no comments.
usr/src/cmd/zoneadmd/vplat.c
2515: nit: s/netmask interface/netmask the interface/
usr/src/lib/libbsm/common/adt.c
usr/src/lib/libinetcfg/common/inetcfg.c
usr/src/lib/libinetutil/common/ifaddrlist.c
Reviewed; no comments.
usr/src/lib/libinetutil/common/ifaddrlistx.c
79: not that I care much, but why alloca here? I thought most
avoided it unless it was needed or obviously safe.
98: leaks 's'.
usr/src/lib/libinetutil/common/inetutil.c
Reviewed; no comments.
usr/src/lib/libinetutil/common/libinetutil.h
59-61: the arguments could be const; the functions don't modify
storage.
96: s/third/fourth/
98: this whole `ifaddrlist' thing seems unwise to me. If we're
going to use BSD functions, we need to follow the BSD definitions of
those functions. Otherwise, we need to come up with our own names.
Having a function with the same name as something in BSD but having
a different set of arguments and different functionality looks like
a recipe for disaster to me.
usr/src/lib/libinetutil/common/mapfile-vers
usr/src/lib/libnsl/nss/netdir_inet_sundry.c
Reviewed; no comments.
usr/src/lib/libsocket/inet/interface_id.c
147: while here, please fix this; bump the count up and (if
possible) add the redo logic. I doubt anyone's going to touch this
stuff again soon.
usr/src/lib/smbsrv/libsmbns/common/smbns_dyndns.c
Reviewed; no comments.
--
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
