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/
The next batch; three more sections covered. I hope to get two more
sections (userland core and boot/DR support) in the last round. I'm
not planning to do kernel core, as that's already got enough eyes.
+++ IPMP ARP support
usr/src/uts/common/inet/arp.h
193: why +1?
usr/src/uts/common/inet/arp/arp.c
158-162: it might be simpler to use arl->arl_ipmp_arl = arl instead
of NULL to signal "this is not in a group." (But perhaps not worth
the effort at this point.)
620: I don't think I understand this logic. Why delete on
ace_xmit_arl match? Shouldn't that just cause us to spin the
ar_ipmp_lookup_xmit_arl wheel instead to pick a new output?
If ace_arl matches and ace_xmit_arl is non-NULL, then why not
promote the latter by doing this?
ace->ace_arl = ace->ace_xmit_arl;
ace->ace_xmit_arl = NULL;
That would effectively kick the ace back out of the group. You'd
only need to delete the ace if both ace_arl and ace_xmit_arl fall to
NULL.
The full code might look something like:
if (ace->ace_xmit_arl == arl && (ace->ace_arl->arl_flags & ARL_F_IPMP))
ace->ace_xmit_arl = ar_ipmp_lookup_xmit_arl(ace->ace_arl,
ace->ace_hw_addr, ace->ace_hw_addr_length, NULL);
if ((ace->ace_arl == arl && ace->ace_xmit_arl == arl) ||
ace->ace_xmit_arl == NULL) {
ace->ace_flags &= ~ACE_F_PERMANENT;
ar_ce_delete(ace);
} else if (ace->ace_arl == arl) {
ace->ace_arl = ace->ace_xmit_arl;
}
631: this could use a different name, as it's really the handler for
an arl leaving the group.
640-645: the comment describes what the code does, but it's unclear
why it does this. Shouldn't it be safe to leave all of these aces
alone? They're already completely committed to the arl that's
leaving the group. Why delete some of them?
644: suggest "!(ace->ace_flags & ACE_F_PERMANENT)" as more readable.
654-686: if we just did 'ace->ace_arl = arl;', would that be enough?
870-872: under what conditions do we ever have to update an
unresolved entry on some arl in a *different* group? It seems to me
that the correct logic should be just this:
If the resolving ARL is in the same group as a matching ACE's
ARL, then update the entry. Otherwise, make no updates.
944-955: this leaves an existing timing hole open. If we move an
address from one arl to another, and we get one of our old ARP
messages echoed back after the switch, we'll misidentify it as being
a failure. It'd be a bit better if we could check whether the
source hardware address matches any of ours and just ignore the
message if so.
992: as above, I don't think that updating unresolved entries for
other groups makes sense.
1163-1176: the suggested changes to ar_ce_delete_per_arl would make
this extra walk unnecessary.
2224: I think we assume elsewhere that the string must be less than
LIFNAMSIZ, don't we? I'm not sure this is needed anyway, as the
callee just uses this for strcmp, and that's bounded by the arl_t's
actual name. We can't run off the end.
2238,2262: these aren't needed. The arguments are used.
2495: I don't think this does anything.
3086: why did 'arl' get removed here? It seems like it's handy as
we dereference ace->ace_arl multiple times.
3599: should this be client_arl or something else?
3601-3603: I don't see how this code is reachable.
usr/src/uts/common/inet/arp_impl.h
Reviewed; no comments.
+++ ipmpstat
usr/src/cmd/cmd-inet/usr.sbin/ipmpstat/ipmpstat.c
150: should note here that these are bit flags, and not just
options. (Otherwise, the explicit assignments look strange.)
194,1288-1304: it seems a shame that the libc version of this
function isn't good enough.
551,733,747,761,778,793,797,802,817,821,826,840,855: should this
return "?" in buf? (Unclear on '--' versus '?' usage.)
567: it doesn't seem like this 'active' buffer is needed. You could
just write into 'buf' directly.
599: I don't think this should have been strlcat; strlcpy seems more
appropriate for the first thing that writes into 'buf.'
732,746,760,777,796,801,820,825,839,854: nvlist_* functions don't
set errno, so this warn() call won't print the right error.
793,817: no warning?
843,858: what if the value is zero?
869-870: not sure what this means. (I think it refers to sysevents
as 'GPECs' ... but I'm not sure what the comment is trying to say.)
986: why do we display v4 targets if both are disabled? The comment
above doesn't seem to explain what's going on here.
1029: ofmt_head could be NULL here; does this need a die() check
like line 1066?
1098: nit: equivalent to just "%s" or simply use puts() and
eliminate line 1102.
1163: similar to 1098; '-' doesn't do anything here.
1183: I think we're guaranteed at this point that valwidth < width.
If it weren't, then line 1177 would have been true.
1282: why both gettext here and a .xcl file?
usr/src/cmd/cmd-inet/usr.sbin/ipmpstat/ipmpstat.xcl
General: not sure I'm wild about this sort of 'exclude' list. It'll
get stale and grow weird. I know gettext() is irritating, but it
seems a little less bad to me.
+++ IPMP API enhancements
usr/src/lib/libipmp/common/ipmp.c
usr/src/lib/libipmp/common/ipmp.h
Reviewed; no comments.
usr/src/lib/libipmp/common/ipmp_admin.c
67: nit: should probably set errno only if result.me_mpathd_error ==
IPMP_FAILURE.
usr/src/lib/libipmp/common/ipmp_admin.h
usr/src/lib/libipmp/common/ipmp_mpathd.c
usr/src/lib/libipmp/common/ipmp_mpathd.h
Reviewed; no comments.
usr/src/lib/libipmp/common/ipmp_query.c
151,179: nit: s/times/time/
172,173,199,200: is it possible for these to be non-NULL at this
point?
1076: may need a comment here that the lack of a
ipmp_read*info_lists call in this case means that we don't have to
bother with freeing anything.
usr/src/lib/libipmp/common/ipmp_query.h
136,153,154: won't it_name just duplicate what's already in if_name?
usr/src/lib/libipmp/common/ipmp_query_impl.h
usr/src/lib/libipmp/common/llib-lipmp
usr/src/lib/libipmp/common/mapfile-vers
usr/src/uts/common/sys/sysevent/eventdefs.h
usr/src/uts/common/sys/sysevent/ipmp.h
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
