> > 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/
>
> Here are some comments on in.mpathd and IPv6 source address selection.
> We've already addressed some of the in.mpathd issues in person. I also
> reviewed the in.ndpd and in.routed changes and had no comments.
Thanks for your feedback Seb; my responses are below. I've also put
together a webrev with the accepted changes (they also include some other
changes for Thiru; sorry):
http://zhadum.east/ws/clearview/clearview-ipmpdev/webrev
> usr/src/cmd/cmd-inet/usr.lib/in.mpathd/Makefile
>
> * 45: Does "-lc" really need to be explicitly included in LDLIBS?
(This is unrelated to my changes.) Also requested by Jim, and already
removed.
> usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_main.c
>
> * 362: We don't care about failure here?
The only alternative is to have in.mpathd stop tracking the IP interface,
and that seems worse since in many cases the local address list is not all
that important. If you like, I could restore the warning message in the
old code, but the "continue" in the old code would actually make things
worse by skipping the IP address part-way through processing, and then
carrying on without the IP address in the local address list anyway.
> * 362: I find it odd that in.mpathd keeps a linked-list of all of the
> local addresses on the system for the purpose of verifying if a
> probe target is a local address. The comment above own_address() in
> mpd_probe.c actually suggests a (IMO) better method, which is to
> simply call SIOCTMYADDR instead.
(This is unrelated to my changes.) IIRC, this code was originally added
to deal with an escalation where in.mpathd would end up probing other
instances in shared-stack zones which was utterly worthless for failure
detection (this was before exclusive zone support). SIOCTMYADDR returns
"no" for IFF_UP addresses in other shared-stack zones, so this approach
was implemented. Of course, now exclusive zones, LDOMs and Xen make
things a lot more difficult and so far the only solution we have for
customers is to tell them not to use multicast for probing. But again,
none of this has much to do with Clearview IPMP.
> * 1872: I think there is potential to grab an IRE_INTERFACE for an
> interface that's not part of the same group as the output interface
> of the "rp" route, since the next-hop is link-local, and every
> interface on the system will have a link-local IRE_INTERFACE route.
> Somehow, this needs to check that the rp1 is associated with the
> same group as rp.
Indeed; thanks for catching that. Fixed.
> * 1872: On a related note, I'm not sure that this prefix_equal() call
> here does anything particularly useful given that the test addresses
> are always link-local and the probe targets also need to be
> link-local. As a result, any link-local next-hop will be good
> enough as a target, and any link-local IRE_INTERFACE will work.
Also changed in the new code.
> * 2483: This is simply !IN6_IS_ADDR_LOOPBACK(), and you don't need
> "loopback_addr".
(This is unrelated to my changes.) Sure, changed.
> usr/src/uts/common/inet/ip/ip6_if.c
>
> * 2361: I can try and guess why you've decided to restrict the
> candidate set to the IPMP ill; is it so that you don't look at
> underlying ill's?
Yes. It's also because I've never found the walk-all-ills aspect of IPv6
source address selection to make much sense, but I realize it's part of
the way IPv6 works.
> This code leaves out other valid ill's for IPv6
> source address selection. The idea of the algorithm is that
> addresses on all IPv6 ill's on the system are in the candidate set
> of IPv6 source addresses, but that addresses on the output ill are
> preferred over those that are not (by virtue of one of the selection
> rules). If the intent is to leave out underlying ill's from the
> candidate set, then I'd be more comfortable with skipping them in
> the ill-walking loop.
Sure; changed.
--
meem
