Peter Memishian writes:
> > I think it might have been been better to include some of the user
> > space removals as well. I assume that the above ioctl does in fact go
> > away with the final version of code, right?
>
> It does. In retrospect, it might've been worth doing the same thing with
> userland that we did with the kernel, though for the most part there's
> either just removed functionality (e.g., no explicit failover/failback
> logic) or new functionality (ipmpstat and supporting infrastructure),
> rather than a rewrite.
OK; not a problem.
> > > > usr/src/uts/common/inet/ip/ip6_ire.c
> > > > 2095-2096: should this be an assertion, or maybe just removed? The
[...]
> > Why bother asserting inside the lookup functions that ill_t isn't NULL
> > if the callers just paint around that check by turning off the flag
> > when it is?
>
> I think the idea is that the caller is at least supposed to think about
> the case, but I agree it's a bit bondage-and-discipline.
The thing that bugs me in this case is that I can't tell that the
caller necessarily *did* think about the case. The code looks like
something that just papers over the issue by turning on the flag only
when the pointer is non-NULL.
If the caller did think about the case, I would have expected that the
comment would explain it, as the choice is certainly not obvious by
inspection. Maybe something like this:
/*
* We see a NULL ire_ipif here when looking up XXX, and looking up
* on an arbitrary ill_t is ok in this case because we expect YYY.
* Otherwise, it's normally the case that ire_ipif is non-NULL, and
* we use MATCH_IRE_ILL to stay on the same ill_t.
*/
The previous comment didn't really explain the situation at all.
Instead, it had this fairly inscrutible remark:
* ip_newroute_v6 calls ire_ftable_lookup with MATCH_IRE_ILL only
* for on-link hosts. We should never be here for onlink.
* Thus, use MATCH_IRE_ILL_GROUP.
I don't see how that relates to the code, so we might be better off
without it, but I'm still unclear on what this really does, or why the
behavior is conditional.
No, it's not *your* problem, but the code is kind of funky here, and
other than a simple search-and-replace of MATCH_IRE_ILL_GROUP, it's
unclear (to me at least) what your change means, other than maybe
"equivalently as strange as before."
> > The new code seems to assume that either this configuration is never
> > used or that if it is used, then the first ipif_t we encounter is
> > always sufficient.
>
> Yes, it's assuming that's true when you ask the kernel to do the selection
> and the subnet matches your destination -- and I can't see a reason to
> have it work otherwise. That said, the Clearview IPMP source address
> algorithm treats the ipif list as circular and starts where it last left
> off (for IPMP ills) to "randomize" the source address selection. It'd be
> straightforward to do this for normal ills too, if need be.
OK.
Unless there's an IFF_PREFERRED or something to control selection, it
sounds to me like making them work the same would be simpler and
easier to understand.
> > > > usr/src/uts/common/inet/ip/ip_ndp.c
> >
> > > > 2755-2770: this code doesn't seem to do much. Maybe this whole
[...]
> > Really? OK. The only thing I see here is ip6i cruft that appeared to
> > be related to the old attach_ill bits, and I though that was mostly
> > being nuked.
>
> The attach_ill stuff is gone, but ip6i_t as a whole remains as it's used
> for other things. One of those other things is putting ND solicitations
> for IPMP probe packets at the head of the queue -- and to do that we need
> to identify that the IP packet as a probe using the ip6i_t.
Ah, ok. I guess I wasn't expecting to see that feature return here,
but I guess it makes sense. Understood.
--
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
