[ Sorry for the delay in following up. As in the past, I've elided issues
that seemed to require no further discussion. ]
> > > > > usr/src/lib/libsocket/inet/interface_id.c
> > > > >
> > > > > 147: while here, please fix this; bump the count up and (if
> > > > > possible) add the redo logic. I doubt anyone's going to touch
> > this
> > > > > stuff again soon.
> > > >
> > > > The function is junk, as is if_nameindex(). I'd want to rewrite them
> > > > rather than tweak them, but that's just too far out-of-scope for IPMP.
> > > > I'll file a CR if you'd like though.
> > >
> > > I wasn't suggesting a rewrite, but rather a minor sprucing as long as
> > > you're in the neighborhood. There's no reason these functions should
> > > be allowed to fail just because plumbing activity is going on at the
> > > same time the call is made.
> >
> > I've tried three times now to go in and "spruce" it up, and I can't do it.
> > It always ends up becoming a rewrite, and then I have to abandon ship.
> > My offer to file a CR is still open :-)
>
> Adding "+ 10" on line 147 so that numifs isn't so likely to be too
> small leads to a rewrite? I don't quite follow. I agree that the
> code needs more help than this ... but I don't see how the sweater
> unravels from that one line.
As we discussed in person, I'm fine with adding "+ 10"; I thought you were
requesting enhancing it to generally handle changes to the IP interface
set, which is a much bigger set of changes.
> > > > > usr/src/cmd/rcm_daemon/common/ip_rcm.c
> > > [...]
> > > > > 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).
> > >
> > > If lint's argument checking doesn't work with the normal gettext
> > > stuff, then I think that's a bug. We should have the necessary
> > > #define in a central place, not littered in source files.
> >
> > I could file a CR to have something added to <libintl.h>, if you like.
>
> Yes, please. This just looks bogus to me.
Filed 6765949.
> > > > > +++ IPMP userland core
> > >
> > > > > usr/src/cmd/cmd-inet/usr.lib/in.mpathd/mpd_tables.c
> > > [...]
> > > > > 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".
> > >
> > > If that did happen ("local-mac-address?=false", maybe), how would the
> > > user recover?
> >
> > They'd need to either put it into another group or change the hardware
> > address through ifconfig.
>
> How could you put it into another group if you can't detach it?
Placing an interface into another group doesn't require a detach.
> > > > > usr/src/uts/common/inet/arp/arp.c
> > >
> > > > > 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?
> > > > >
> > > >
> > > > Because if there's an ace_xmit_arl match in the non-IPMP case, then
> > > > ace_arl will also match (no change in behavior), and if there's an
> > > > ace_xmit_arl match in the IPMP case, then this is an intentional
> > purge of
> > > > the ACE triggered by an unplumb of an IP interface in a group, which
> > IP
> > > > controls. For instance, if this is an ACE_F_PERMANENT ACE, then
> > after the
> > > > ar_ce_delete_per_arl(), IP will ask ARP to create a new ACE for that
> > IP
> > > > address with a different ace_hw_addr. There's no sense in having the
> > old
> > > > ACE_F_PERMANENT hanging around with a NULL ace_xmit_arl and a stale
> > > > ace_hw_addr.
> > >
> > > OK ... I guess what's unclear to me is when IP is in control of the
> > > interface selection and when the ARP module does it. It seems like
> > > there are cases where the ARP module can do this on its own.
> >
> > Basically, for ACE_F_PERMANENT entries, IP is in control of the creation
> > and interface selection by passing AR_ENTRY_ADD messages that have a
> > hardware address specified.
>
> What has me confused here is the logic in ar_ipmp_lookup_ipmp_arl. It
> seems that the ARP module is trying to help out IP by trying several
> different means to find a usable arl_t -- first MAC address, then IP
> address, and then finally just "any," but that we do this only once.
>
> Why is it ok for this function to choose a different arl_t than the
> one specified by IP, but not ok for ARP to choose a different arl_t
> when needed?
This function works in concert with IP. That is, IP is aware of ARP's
algorithm in ar_ipmp_lookup_ipmp_arl(), and in fact it depends on it
so that it can control ARP's arl selection through its request to ARP.
In general, if the arl needs to change, IP needs to request that ARP
make this change; ARP is not allowed to change it by itself because
it does not have a sufficient view of the system to do that correctly.
>
> > > > > 654-686: if we just did 'ace->ace_arl = arl;', would that be
> > enough?
> > > >
> > > > I don't think it's appropriate to always keep these ACEs -- e.g., if
> > it
> > > > was an ACE for a proxy ARP entry on the IPMP interface, we don't want
> > that
> > > > ACE to linger behind on the removed interface. (Note that all we're
> > doing
> > > > here is ar_ce_delete(); the comments make the code seem more
> > daunting.)
> > >
> > > What bugs me is that we're removing entries (possibly "permanent"
> > > ones) that are on the upper-level IPMP interface, and we're removing
> > > them only because the currently selected lower-level interface went
> > > away.
> >
> > But that went away because IP told us it was going away, and since IP
> > knows what ACE_F_PERMANENT entries it put where, it's smart enough to know
> > how to recreate them. (This ties in with my "subtle cases" comment above.)
>
> This doesn't have any exceptions in it so that it just deletes
> ACE_F_PERMANENT or things controlled by IP. It deletes everything,
> including outstanding queries (and the queued packets on them), static
> ARP entries set up by a user, entries trying to do DAD, and learned
> addresses.
Static entries set up by a user will be repopulated by IP through the
IPMP-aware SIOC*ARP logic in IP. The other entries are just cache entries
and will be recreated on-demand.
--
meem
