> > http://cr.opensolaris.org/~meem/clearview-ipmp-cr/
>
> I've finished all but the "kernel core" bits.
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-diff3
> +++ IPMP boot/DR support
>
> usr/src/cmd/cmd-inet/usr.sbin/if_mpadm.c
>
> 47-48,244-286: more duplicates.
Duplicates with what? Unlike the ones that recently got added to libc,
these call gettext(), which means the call sites don't have to get fouled
up with that junk.
> 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.
I agree it's odd, but that's the way the if_mpadm has always worked, and
in theory administrative scripts could've been written with this in mind
(e.g., if non-zero is returned, whatever was requested was not done), so I
left it be.
> 159-163,218-222: this was changed from an actual error case to a
> warning. Why?
There wasn't a strong reason, but the reasoning was that those are just
sanity checks and not needed for a successful offline/undo-offline
operation. I've never seen them trigger though, so if you feel strongly,
I can change them to fatal errors.
> 172: it's odd that this is tested twice -- once at line 162, and
> then again here.
It was trying to catch two different cases (the first being that in.mpathd
didn't do its job, and the second being that something cleared IFF_OFFLINE
-- e.g., another application racing with SIOCGLIFFLAGS/SIOCSLIFFLAGS).
But in truth the two cases aren't always distinguishable; I've removed the
test at line 162.
> 175: nit: no need to do this (and risk the known problems with
> 203: similar issue here: no reason to bring up if it's already
> there.
Based on the arguments passed to our calls to ifaddrlistx(), these
addresses will always be IFF_UP and not IFF_UP, respectively (yes, it
could change after teh call to ifaddrlistx(), but that change will not be
reflected in ia_flags anyway).
> 203: too bad there's no way to remember which ones were
> intentionally down before; this brings them all up.
Yes, though with the new design, this would only affect intentionally-down
test addresses.
> 252-253,275-276: not necessary: strerror guarantees that it doesn't
> return NULL.
Bad habit from the S9 days; fixed.
> 261,284: nit: equivalent to the more obvious 'strchr' in this case.
But what about efficiency? ;-) Fixed.
> usr/src/cmd/rcm_daemon/common/ip_rcm.c
>
> General: I'm going to need a long soak in the tub now.
Yeah, it's pretty awful. The good news is that there's a lot less of it
than there used to be.
> 63-70: negative logic is hard to read (and wtf?).
Not my code, but this is a clever trick to allow lint to do format string
checking (since otherwise the function call obscures the format string for
lint).
> 89-92: ew.
Yeah. Again, not mine.
> 852: this test should be '!offline && numup > 0'
Good catch! This got botched during some recent cleanup in here.
> 864-865: why add the separator _before_ the address? It looks like
> line 866 should come before these two lines, not after.
Agreed; changed.
> 2345-2346: this doesn't appear to be possible.
Unrelated to my changes, but fixed.
> 2375,2473,2480: nit: use malloc.
Unrelated to my changes, but fixed.
> 2383: nit: could use "!grouped &&" to avoid the goto.
We'd also probably want to add that at line 2388, which starts to get
ugly. So I've left it as-is.
> 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.
I agree, though this problem was even worse in the original ip_rcm code
(check out the old "parsing" engine ;-)
> 2437-2443: this is weird.
This code dates back to the old plumb-IPv6-on-demand logic, which no
longer applies in recent Nevada since IPv6 loopback is always plumbed at
boot and never gets unplumbed. As such, I've removed it.
> 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.
Ah. I've added similar logic in the pclose() case now.
> 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?
That logic only works if there was indeed something to bring up. The case
we're concerned about is a config which only has data addresses on the
underlying interface (no test addresses). In this case, all of the data
addresses will have been filtered out by ifparse, so we won't perform any
ifconfig operations at all, and we'd end up erroneously leaving the IP
interface down.
> 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.)
If there's no test address (which is the case we're concerned about), then
there will just be a 0.0.0.0 address instead (the data addresses are all
off on the IPMP IP interface). So I don't think bringing it up will have
a big effect.
> 2553-2556: shouldn't this be done after all of the hostname files
> are read?
When would it matter? Note that any IPMP interfaces are already plumbed.
> usr/src/cmd/svc/milestone/net-physical
>
> 157: this doesn't seem necessary.
I wanted to mirror the logic on lines 175 and 206.
> 158-159,165: easier to read as "for intf in $ipmp_list; do"
As above, I wanted to mirror the logic on lines 177 and 208.
> 186,217,326: nice catch!
Thanks :-)
> 224-225,231: would also be easier to read using 'for'.
Same as above.
> usr/src/cmd/svc/shell/net_include.sh
>
> 580-582: not sure if msglog likes all these open/closes. I'd do
> this as a single echo command, or use a () subshell.
In my testing, I didn't see any problems with it. I tried the two
alternatives you suggested and they were even uglier :-(
> 614,623: simpler as 'for intf; do'
This would introduce an awkward linewrap at line 615.
> +++ IPMP userland core
>
> usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_main.c
>
> 84: nit: initialization not really needed.
Unrelated to my changes, but changed.
> 308: need a fudge factor here.
Unrelated to my changes, but fixed. I also removed the EINVAL
SIOCGLIFCONF handling since that should only apply to OSIOCGLIFCONF.
> 310: nit: cast not needed.
Unrelated to my changes, but removed.
> usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_probe.c
>
> 406: nit: I think it might be necessary to read and discard until
> msg_flags are zero after this event.
Why would there be more than one message marked with these flags? I don't
see any other code that does what you suggest with these flags.
> 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.
Nice catch; fixed.
> 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."
That's not true though -- e.g., if TRACK_INTERFACES_ONLY_WITH_GROUPS=no in
/etc/default/mpathd, then in.mpathd will track interfaces even if they're
not in an IPMP group (and change the flags). However, the comment should
say "if in.mpathd isn't tracking", not "if IPMP isn't tracking"; fixed.
(As an aside, this is unrelated to my changes.)
> 705-707: I can't parse this. (Ability of in.mpathd to do what?)
Yes, there's a missing word -- it should say "of in.mpathd to *track* the
group's interfaces".
> 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?)
It refers to the enclosing function; clarified.
> 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.
True. I'm not sure if it's worth worrying about this case, but I think we
could handle it by adding an explicit "offline reason" argument to
phyint_offline(), and have phyint_offline() do the close even if
pi_hwaddrdup is set as long as the offline reason is not "hwdup".
> 3124: is the fdt the same on every interface, or does it not matter
> which interface we use?
It's the same for every interface in a given group. However, it has no
meaning if no interface in the group has probe-based failure detection
enabled, hence the current logic.
> 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.
It would be less code, but I think the behavior would be surprising, and
there could reasonably be cases in the future where the caller might want
to do something else, like print a message using the contents of that
argument prior to freeing it.
> 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 ... ?
Unrelated to my changes, but I've changed it to only do the initial
assignment (I have a slight preference not to start enums at zero since I
prefer this to be initialized explicitly rather than through calloc().)
> 167,169,221-222: nit: boolean_t used in some cases, bit flags in
> others.
I'm not a huge fan of bitfields, but I'll use them when they're already
there.
> 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.
There are a number of established interfaces already assume -1 has a
pointer representation -- e.g., MAP_FAILED with mmap(2) and SIG_ERR with
signal(3C). So, while a bit ugly, I think it is safe.
> 82: should IFF_IPMP be in this list?
No; IFF_IPMP doesn't indicate application control.
> 2228: module operations are not supported on other virtual
> interfaces, either.
OK, I've added an additional check for IFF_VIRTUAL (in Nevada, it bombs
out when the _I_MUXID2FD is done).
> 4425: this should handle putting in.mpathd in a separate contract so
> that it doesn't share fate with its invoker.
Getting SMF/contracts sorted for IPMP is a separate project (note that
this code just moved from the old setifgroupname() implementation).
> usr/src/cmd/cmd-inet/usr.sbin/ifconfig/revarp.c
>
> 89: could exclude other virtuals here, I guess.
Unrelated to my changes, but I believe that's already caught since vni
will have IFF_NOARP set and IFF_LOOPBACK is checked explicitly.
--
meem
