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/
My last batch. If someone on the SWAN wants a copy of the entire
file, look here:
/home/carlsonj/projects/other/clearview-ipmp.txt
I've finished all but the "kernel core" bits.
+++ IPMP boot/DR support
usr/src/cmd/cmd-inet/usr.sbin/if_mpadm.c
47-48,244-286: more duplicates.
153-154,190-191: should these cases be errors? I can understand
printing a message in this case, but returning with an error as
though something were wrong -- when the user's wishes have been
obeyed -- seems odd.
159-163,218-222: this was changed from an actual error case to a
warning. Why?
172: it's odd that this is tested twice -- once at line 162, and
then again here.
175: nit: no need to do this (and risk the known problems with
setting flags) if the interface is already down.
203: similar issue here: no reason to bring up if it's already
there.
203: too bad there's no way to remember which ones were
intentionally down before; this brings them all up.
238: not necessary; both of the callers print error messages on
failure.
252-253,275-276: not necessary: strerror guarantees that it doesn't
return NULL.
261,284: nit: equivalent to the more obvious 'strchr' in this case.
usr/src/cmd/rcm_daemon/common/ip_rcm.c
General: I'm going to need a long soak in the tub now.
63-70: negative logic is hard to read (and wtf?).
89-92: ew.
476: c style
852: this test should be '!offline && numup > 0'
864-865: why add the separator _before_ the address? It looks like
line 866 should come before these two lines, not after.
2345-2346: this doesn't appear to be possible.
2346,2353, et seq.: this should be returning B_TRUE/B_FALSE.
2375,2473,2480: nit: use malloc.
2383: nit: could use "!grouped &&" to avoid the goto.
2388-2395: this code doesn't quite respect the parsing rules that
ifconfig has. In particular, "set group preferred" would be
misinterpreted as an IPMP interface, rather than interface with IP
address 'group'. (There are obviously other examples with 'netmask'
and 'broadcast' -- anything that can take an argument.)
I guess I'm not terribly worried about getting this code right, but
it seems a little odd to try hard at making sure that 'group' has a
specified argument while ignoring the rest of the syntax.
2437-2443: this is weird.
2523-2525: this doesn't quite work the way you'd expect. When using
popen(), you don't get the actual error code until you do pclose(),
and in this case, the code ignores that error on line 2533. 2522 is
unlikely to fail.
2541-2543: I don't understand why this "however" is true. The code
already detects single-line files and sets stdif so that "netmask +
broadcast + up" is done, so why is an extra "up" needed here? I
suspect that this would break some known Zones-related cases.
(E.g., one interface plumbed and up on the global zone only, another
plumbed but not up in the global zone, with shared-stack non-global
zones using logicals that second interface.)
2553-2556: shouldn't this be done after all of the hostname files
are read?
usr/src/cmd/svc/milestone/net-init
usr/src/cmd/svc/milestone/net-loopback
Reviewed; no comments.
usr/src/cmd/svc/milestone/net-physical
157: this doesn't seem necessary.
158-159,165: easier to read as "for intf in $ipmp_list; do"
186,217,326: nice catch!
224-225,231: would also be easier to read using 'for'.
256: s/plumbed/non-IPMP/
usr/src/cmd/svc/shell/net_include.sh
213: nit: doesn't do anything.
487-488,489,498,568: simpler as 'for'.
580-582: not sure if msglog likes all these open/closes. I'd do
this as a single echo command, or use a () subshell.
614,623: simpler as 'for intf; do'
+++ IPMP userland core
General: looks like 9 files, not 8.
usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_defs.h
Reviewed; no comments.
usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_main.c
84: nit: initialization not really needed.
308: need a fudge factor here.
310: nit: cast not needed.
usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_probe.c
246: we now have htonll
406: nit: I think it might be necessary to read and discard until
msg_flags are zero after this event.
713: nit: should also print new recv_tvp value.
1422,1423: nit: use 'else if'
usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_tables.c
251,272: not clearing out pi_dh in the open failure case could lead
to a double free.
532: calloc handles this.
683-697: it's true that nobody else should be using these flags, but
I'm not sure it makes sense for in.mpathd to change flags on
interfaces where the group name is not set. To me, group name set
means "owned by IPMP," while group name clear means "not owned by
IPMP."
705-707: I can't parse this. (Ability of in.mpathd to do what?)
705: this statement is unclear. If the group creation fails, we
bail out, we don't continue on. (Maybe the "this routine" is
reference is ambiguous; does it mean the calls below or the
enclosing function?)
1384-1387: nit(?): it looks like you can't detach an interface with
a duplicate MAC address. I guess that shouldn't happen, but it
seems like an odd corner case.
1410: I think '!(pi->pi_flags & IFF_INACTIVE)' is easier to read.
1433: note that pi_dh can be non-NULL but point to freed memory at
this point; on next entry, we will fall through and not try to
reopen. (See first issue about line 251.)
3124: is the fdt the same on every interface, or does it not matter
which interface we use?
3367,3369,3380,3382,3396,33987: this could be simplified. In every
case that you call ipmp_snap_add*, you always free on error. This
means that the ipmp_snmp_add* functions could be designed to consume
the pointer unconditionally.
usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_tables.h
144-146: why do these need to be assigned explicitly? Don't enums
work well enough ... ?
167,169,221-222: nit: boolean_t used in some cases, bit flags in
others.
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/defs.h
Reviewed; no comments.
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.c
25: this is a little scary, as there's no guarantee that -1 has a
representation as a pointer. I suggest something like this instead:
static char stubloc;
#define IPMPSTUB (void *)&stubloc
Or, if you prefer:
static const char ipmpstub[] = "ipmpstub0";
#define IPMPSTUB (void *)ipmpstub
You're guaranteed that static locations of storage have distinct
addresses and can't be equal to any other pointers.
79: nit: could be const.
82: should IFF_IPMP be in this list?
2228: module operations are not supported on other virtual
interfaces, either.
4425: this should handle putting in.mpathd in a separate contract so
that it doesn't share fate with its invoker.
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/ifconfig.h
Reviewed; no comments.
usr/src/cmd/cmd-inet/usr.sbin/ifconfig/revarp.c
89: could exclude other virtuals here, I guess.
--
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
