Hi Meem,
Here is some of the nits from phase 1 review.
-------------------------------------------------------------------------------------------------------------------------
usr/src/uts/common/inet/ip.h
774-784: No extra spaces are needed in the bulk comments section as the
second
section is removed.
usr/src/uts/common/inet/ip/igmp.c
Do we need ipsec_out_t in igmp_sendpkt and igmpv3_sendrpt, if we are not
using ipsec_out_attach_if?
Is it also used in new implementation?
I have checked this after talking to you on friday, and it is used in
ip_wput_ire for ipsec_out_dontroute and ipsec_out_multicast_loop. So,
this is just an FYI.
usr/src/uts/common/inet/ip/ip.c
Some of the IPMP related ioctls are obsoleted. What about the userland
applications which are using them?
Ex: rcm_daemon (MI_SETOINDEX option which will use SIOCSLIFOINDEX).
usr/src/uts/common/inet/ip/ip_if.c
Line: 13494
Extra "when" in the comments.
usr/src/uts/common/inet/ip_ire.h
Line: 97
+/* UNUSED 0x1000 */
I guess it should be UNUSED 0x0400 - 0x0800.
-----------------------------------------------------------------------------------------------------------
Thanks,
Ramesh.
Peter Memishian wrote:
> At long last, we're ready to start the Clearview IPMP code review.
> The review is divided into two phases:
>
> * Phase 1 covers the removal of the existing Nevada IPMP kernel
> implementation. We're starting this now.
>
> * Phase 2 covers the Clearview IPMP kernel implementation, plus all
> userland changes. This is the real meat of the project, and will
> start in a couple of weeks.
>
> If you're interested in reviewing either or both phases, please let us
> know, and please feel free to dig into Phase 1 (details below). Note that
> there will only be one integration -- the Phase 2 codebase. However,
> because the Clearview IPMP kernel implementation is entirely[1] new, we've
> elected to split up the review to ensure Phase 2 reviewers will not be
> distracted by meaningless diffs against the old (and irrelevant) Nevada
> IPMP kernel implementation.
>
> The remainder of this message discusses Phase 1; we'll provide details for
> Phase 2 separately once it's ready.
>
> As stated above, Phase 1 covers the removal of the Nevada IPMP kernel
> implementation. While this is a large amount of code change (roughly 9500
> lines), the lion's share of it is code removal or comment rework. Indeed,
> there are less than 150 new lines of code in this wad. As such, we
> believe a week and a half is enough time for this part of the review (we
> will be allocating twice as much time for Phase 2).
>
> Please be aware that the IPMP Phase 1 sourcebase is itself based on the
> Clearview IP Observability sourcebase, which in turn is based on the
> latest Nevada. This has been done for several reasons, but especially
> because Phase 2 provides IPMP IP observability support, which is only
> possible with the Clearview IP Observability bits. Since IP Observability
> is also scheduled to integrate into Nevada before Clearview IPMP, the
> code review also matches what we plan to integrate (if this changes, we
> will of course provide diffs for review). Also, note that the actual
> impact of this is quite small because the changes to the IP sourcebase for
> IP Observability are minimal (mostly some changes to the hooks and some
> multicast kernel API enhancements).
>
> Because IPMP integrates deep into IP, the removal of IPMP means more
> than just weeding out the obviously-unused functions. For instance, the
> the IPSQ codepaths have been noticeably simplified since IPSQ<->ill
> bindings can no longer change, and all the special-case ifindex handling
> for multicast is no longer necessary. In general, we have tried to be
> thorough in eradicating all tentacles of complexity that were due to the
> Nevada IPMP implementation, but there was a lot of code to sift through,
> and we would welcome more eyes. In some corner cases, we have chosen to
> leave behind certain aspects of the original design because they were
> useful in the new codebase as well, but those are relatively rare and
> hopefully will not impede anyone's quest :-)
>
> Finally, Phase 1 also includes a collection of small formatting and
> comment fix-ups to the Nevada codebase that proved useful to the Phase 2
> codebase but that we didn't want cluttering up the Phase 2 diffs.
>
> With that background, the webrev is available at:
>
> http://cr.opensolaris.org/~meem/clearview-noipmp-cr/
>
> A stable internal Mercurial repo is available at:
>
> /net/zhadum.east/export/ws/clearview/clearview-noipmp-cr
>
> Finally, a stable internal cscope is available at:
>
> /net/zhadum.east/export/ws/clearview/clearview-noipmp-cr/usr/src
>
> Thanks and have fun!
>
> [1] OK, we did reuse a couple of ioctl entrypoints ;-)
>
