> > 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.
Great, thanks! As before, I've elided things that required no further
discussion. A webrev of the diffs is at:
http://cr.opensolaris.org/~meem/clearview-ipmp-cr-diff2
> 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.
Yeah, I was thinking about SNMP too. It seems like we'd want to get that
folded into the upstream net-snmp sourcebase once LIFC_UNDER_IPMP support
is in OpenSolaris. I'd be happy to take that on.
> What else (possibly outside of ON) should use that flag?
I'd suspect some third-party IP monitoring/administration utilities will
benefit from it, but I didn't have anything specific in mind.
> +++ IPMP-aware userland changes
>
> 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.
In that case, the administrator is expected to explicitly configure any
probe targets -- e.g., by using dhcpinfo. As you know, I regard the fact
that the DHCP client directly configures routes at all as a bug.
> 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.)
As you may recall, we decided to have dhcpagent no longer monitor changes
to IFF_UP (this will be included in the PSARC commitment materials). This
change was necessary so that the DHCP client is not affected by an offline
operation (since offline brings down test addresses). The old IFF_DUPLICATE
logic in interface.c is now the standard case, so that code block is no
longer needed.
> usr/src/cmd/cmd-inet/sbin/dhcpagent/request.c
>
> 1010: nit: is the test of pif_under_ipmp needed here?
We could rely on pif_grindex being zero (since no IP interface will have
index zero), but it seemed clearer to me as-written.
> 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?
Right; changed.
> 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.
By adding a `length' argument?
> 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;
Indeed, but I think it could be done as:
packetlen += add_opt_lla(pi, (struct nd_opt_lla *)pptr);
pptr = &packet[packetlen];
> 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.)
No, it arrives with this wad. The check here is because this (highly
questionable) loop is trying to choose a default datalink (not IP
interface) to snoop on. Since IPMP has no datalink, we skip IPMP IP
interfaces. Ultimately, snoop should use dladm_walk() to do this, not
SIOCGLIFCONF. But changing that seemed out-of-scope.
> 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?
I agree that this shouldn't be necessary; I've reverted the change.
> 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.
Just following the same logic as ifaddrlist(). It could be a problem in a
multithreaded program if there are lots and lots of IP addresses, though,
so I'm willing to change both if you feel strongly.
> 98: leaks 's'.
Fixed by restructuring.
> usr/src/lib/libinetutil/common/libinetutil.h
>
> 59-61: the arguments could be const; the functions don't modify
> storage.
Changed. I also removed sockaddrzero() since nothing used it.
> 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.
I agree, except that fork already happened when we added the second
argument (address family) during IPv6 development. Given that
compatibility has already been compromised and this function is
consolidation-private, it seemed OK to me to add the `flags' field.
> 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.
The function is junk, as is if_nameindex(). I'd want to rewrite them
rather than tweak them, but that's just too far out-of-scope for IPMP.
I'll file a CR if you'd like though.
--
meem
