> > 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 first batch of review comments; three sections covered. After I
> get back from vacation, I'll hit the others.
Great, thanks! I've elided your comments when I've taken your suggestion
and no additional discussion was needed.
A webrev showing the changes is at:
http://cr.opensolaris.org/~meem/clearview-ipmp-cr-diff1
A number of your comments relate to the symlink/lintlib/headers for
libipmp. The libipmp library is Contracted Consolidation Private to the
Sun Cluster consolidation, which is historically why we've provided this
stuff; changing this seemed unrelated to Clearview IPMP and thus we've
continued to provide those things, albeit in /lib instead of /usr/lib
(because we had to move several IPMP programs into the root filesystem).
Yes, we could provide a separate internal package; we could also do that
in Nevada -- again, it seems like a separate bit of work and I'd rather
not do it with this wad.
The backward links for /usr/lib to /lib are a separate matter; I agree
they should not be necessary and I have removed them.
> General:
>
> I'm guessing that the new 'SOL_ROUTE' thing was discussed on the
> Clearview mailing list. I can't find any other discussion of it,
> including in ARC materials, though I did try to grep around for it a
> bit.
As you found, it was introduced in response to Erik Nordmark's feedback.
Yes, we could probably use SOL_SOCKET, but this seemed more appropriate
to me. It will be covered in the commitment materials for 2007/272.
> The RT_AWARE name seems a little vague ... RTS_ADDRTYPES or
> RTS_IFTYPES or some such might be more accurate.
It's intentionally general, in that it's expected to generally convey the
consumer's "awareness" with respect to routing socket events. It could be
expanded in the future to cover other LIFC_* flag cases.
> usr/src/cmd/cmd-inet/usr.lib/in.mpathd/Makefile
>
> 62-63: this looks reasonable to me, but you may need to test upgrade
> cases before integrating. The packaging tools have historically had
> fits dealing with files that change into links and vice-versa.
In the past, we changed /sbin/in.mpathd from an executable to a symlink
and I don't recall any issues. I'm less sure about the symlink ->
executable case. In any case, I plan to test upgrade.
> usr/src/cmd/rcm_daemon/Makefile.com
>
> 123: what does '-L$(ROOT)/lib' do here? Doesn't the right thing
> happen by default?
It doesn't; LDLIBS_MODULES doesn't automatically include the library
search path. I don't know why they did it this way.
> usr/src/pkgdefs/SUNWcsr/prototype_com
>
> 405: nit: I'm not sure if it makes sense to have this in the
> administrator's search path, as there's nothing the administrator
> ought to be doing with it. "/lib/inet/in.mpathd" would be slightly
> better.
It's been in the administrator's search path since S8. Further,
unfortunately, third-party software is aware of this location
(and the new location) (e.g., to restart it by hand).
> usr/src/pkgdefs/SUNWcsu/prototype_com
>
> 634: why is this required? /usr/lib/inet/in.mpathd is a private
> interface, so nothing else ought to be using this. (Right?)
See above.
> usr/src/uts/common/Makefile.files
>
> 482: nit: having ipmp.o first looks a little odd; it doesn't sort
> before igmp.o ... not that the rest of the list is in great shape.
Leftover that was for personal convenience during development; fixed.
> +++ dlpistub
>
> usr/src/uts/common/inet/dlpistub/dlpistub.c
>
> 129: this appears to leak the minor ID (allocated at line 118).
Indeed; I reordered things to avoid having to do cleanup.
> 169: nit: wput doesn't have to return anything. (Yeah, I realize
> that's part of your text-line-conserving return-for-error code.)
Yes; I considered both and prefer the way it is.
> usr/src/uts/intel/os/name_to_major
> usr/src/uts/sparc/os/name_to_major
>
> Does renaming an existing major number work? (I've never tried that
> before; it could need some upgrade tests.)
Will test.
> usr/src/uts/common/inet/ip/rts.c
>
> 560,562,711,713: nit: I don't think these locks do much of anything.
Yeah it's a little weird, but status quo for conn_lock and options --
e.g., see ip_opt_set().
> usr/src/uts/common/io/ib/mgt/ibcm/ibcm_arp_link.c
>
> 477: EFAULT sure is odd ... but I guess it's ok.
I agree. Unfortunately, the IB test suite seems to depend on some of
these weird errnos (there's precedent for this one at line 474 of the
original file).
> usr/src/uts/common/net/if.h
>
> 174: instead of "traditional interface," I suggest saying "any
> interface." (The former makes it sound as if there are interfaces
> on which the user can change some of the IFF_CANTCHANGE flags from
> user space ... and that's not true.)
Except that they can before SIOCSLIFNAME is set, but that's another matter
:-O
> 601: is this equivalent to "gi_nv4 != 0"?
> 602: and is to "gi_nv6 != 0"?
No. Perhaps the comments are too terse but, gi_v4 and gi_v6 represent
whether the IPMP group interface is plumbed for IPv4/IPv6, whereas the
gi_nv4 and gi_nv6 counters represent the number of underlying IPv4 and
IPv6 interfaces. For example, if one does "ifconfig ipmp0 ipmp", then
gi_v4 will be B_TRUE, but gi_nv4 will be zero.
> 603-606: why are some set to uint32_t and one is set to uint_t?
No good reason; I've removed the use of uint32_t.
> 603: if I want this value, can I just add gi_nv4 and gi_nv6?
Yes -- and it turns out nothing uses it, so I've removed it.
> usr/src/uts/common/net/route.h
>
> 265-266: I don't think I agree with using the RTA_.* name space
> (which is currently used for the rtm_addr bit mask values) for this
> new purpose. It'll get confusing in applications, and invites
> error. Please come up with a new name (RTAA_.*?).
I went with RTAW_
> usr/src/uts/common/netinet/in.h
>
> 491,537-549: this appears to be a duplicate of the existing
> IN6_IS_ADDR_MC_SOLICITEDNODE() macro. I think you can just revert
> this file.
Thanks for that -- I was surprised it wasn't already there ;-)
> usr/src/uts/common/sys/sockio.h
>
> 238: curious ... if we can remove them safely, why can we not reuse
> them?
Because we'd like any lingering consumers to fail, rather than silently
take on new behavior.
--
meem
