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 first batch of review comments; three sections covered. After I get back from vacation, I'll hit the others. 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. Previously, there was only one socket option that was specific to routing sockets (SO_USELOOPBACK), and I *think* the intention might have been to allocate more SO_* things if necessary. But I guess I can live with it. The RT_AWARE name seems a little vague ... RTS_ADDRTYPES or RTS_IFTYPES or some such might be more accurate. Judging based on the July 2007 two-message exchange between you and Erik on clearview-discuss (the only record I can find), it sounds like you intend this to be a documented interface. If so, then this will need ARC review. +++ packaging/build usr/src/Targetdirs 917,1039-1040, et seq.: nit: unless you're moving a public library from /usr/lib/ to /lib/, I don't think this is necessary. 1039-1040: is this intended as a public library? usr/src/cmd/cmd-inet/usr.lib/in.mpathd/Makefile 45: nit: do we really have to say "-lc" for an ordinary executable? 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. usr/src/cmd/cmd-inet/usr.sbin/Makefile 149: mismerge; remove this line. usr/src/cmd/cmd-inet/usr.sbin/ifconfig/Makefile Reviewed; no comments. usr/src/cmd/cmd-inet/usr.sbin/ipmpstat/Makefile 31: nit: why is "-lc" needed here? usr/src/cmd/rcm_daemon/Makefile.com 123: what does '-L$(ROOT)/lib' do here? Doesn't the right thing happen by default? usr/src/lib/libinetutil/Makefile.com usr/src/lib/libipmp/Makefile usr/src/lib/libipmp/Makefile.com Reviewed; no comments. usr/src/pkgdefs/SUNWarc/prototype_com 125-126: I assume this is a private library, and the only reason it's shipped is to help with compiling the test suite, right? If so, then consider moving this to a separate undelivered package. usr/src/pkgdefs/SUNWarcr/prototype_com Same comment. usr/src/pkgdefs/SUNWckr/prototype_com usr/src/pkgdefs/SUNWckr/prototype_i386 usr/src/pkgdefs/SUNWckr/prototype_sparc usr/src/pkgdefs/SUNWcsd/postinstall Reviewed; no comments. usr/src/pkgdefs/SUNWcsl/prototype_com 150-151: these likely aren't necessary. I don't think anything outside of ON should search for them here at run time. usr/src/pkgdefs/SUNWcslr/prototype_com 90: similar comment about the compilation symlink, which isn't supposed to be shipped for private libraries. 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. 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?) usr/src/pkgdefs/SUNWhea/prototype_com 269: very minor nit: we're starting to grow a small forest of ipmp files in the top level. usr/src/tools/scripts/bfu.sh Reviewed; no comments. 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. usr/src/uts/common/Makefile.rules Reviewed; no comments. usr/src/uts/intel/Makefile.intel.shared 220: move up to line 218. usr/src/uts/intel/dlpistub/Makefile 27: nit: stale comment. usr/src/uts/intel/ip/ip.global-objs.debug64 usr/src/uts/intel/ip/ip.global-objs.obj64 usr/src/uts/sparc/Makefile.sparc.shared Reviewed; no comments. usr/src/uts/sparc/dlpistub/Makefile 22,27: nit: stale comments. usr/src/uts/sparc/ip/ip.global-objs.debug64 usr/src/uts/sparc/ip/ip.global-objs.obj64 Reviewed; no comments. +++ dlpistub usr/src/cmd/devfsadm/misc_link.c 100: why here and not down on line 107 with the rest of the networking stuff? (Guess it's ok, but maybe a little odd.) usr/src/uts/common/inet/dlpistub/dlpistub.c 129: this appears to leak the minor ID (allocated at line 118). 169: nit: wput doesn't have to return anything. (Yeah, I realize that's part of your text-line-conserving return-for-error code.) usr/src/uts/common/inet/dlpistub/dlpistub.conf usr/src/uts/common/inet/dlpistub/dlpistub_impl.h usr/src/uts/common/sys/dlpi.h Reviewed; no comments. 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.) +++ IPMP kernel fringe usr/src/cmd/mdb/common/modules/ip/ip.c usr/src/cmd/truss/codes.c usr/src/cmd/truss/print.c usr/src/uts/common/inet/ip/ip_netinfo.c Reviewed; no comments. usr/src/uts/common/inet/ip/rts.c 560,562,711,713: nit: I don't think these locks do much of anything. 716: missing 'default' handling (should return EINVAL, but falls through to return garbage). usr/src/uts/common/inet/ip/rts_opt_data.c usr/src/uts/common/inet/ip_rts.h Reviewed; no comments. usr/src/uts/common/inet/ipclassifier.h 294: nit: why the blank line here? (I think it was here previously to separate the stuff above from the bit flags ... but now it's just an odd blank line.) usr/src/uts/common/inet/ipnet/ipnet.c usr/src/uts/common/inet/tcp/tcp_fusion.c usr/src/uts/common/inet/udp/udp.c Reviewed; no comments. usr/src/uts/common/io/ib/mgt/ibcm/ibcm_arp_link.c 409: nit: revert; initializer not used. 411,414: initializer not used. 477: EFAULT sure is odd ... but I guess it's ok. 514,516: these cannot be NULL here. usr/src/uts/common/ipp/ipgpc/classifierddi.c Reviewed; no comments. 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.) 601: is this equivalent to "gi_nv4 != 0"? 602: and is to "gi_nv6 != 0"? 603-606: why are some set to uint32_t and one is set to uint_t? 603: if I want this value, can I just add gi_nv4 and gi_nv6? 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_.*?). 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. usr/src/uts/common/sys/socket.h 179: this comment is no longer quite true. These are the non-IP- protocol and generic socket options. (SOL_ROUTE doesn't apply to "[the] socket itself.") usr/src/uts/common/sys/sockio.h 238: curious ... if we can remove them safely, why can we not reuse them? -- 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
