On Thu, May 24, 2007 at 05:39:21PM +0900, Keiichi SHIMA wrote:
> Hello all,
> 
> I'm now working on porting the Mobile IPv6 kernel part code developed  
> by the SHISA project (http://www.mobileip.jp/) to NetBSD current as  
> its optional feature.
> 
> I put a tarball of the diff file and some new files as http:// 
> www.mobileip.jp/~keiichi/tmp/mobile-ipv6-20070524.tgz for review.
> 
> I want to import this change to the HEAD tree, however since it is a  
> big change, I would like to ask any kind of comments or suggestions  
> for this project.

The patch sprinkles #ifdef MOBILE_IPV6 / #endif all over the IPv6 code.
That does not help the readability of the IPv6 code.  Surely you can
confine the #ifdefs to fewer places and fewer files?  For example, can you
introduce some predicate that always evaluates to 0 #ifndef MOBILE_IPV6,
instead of the following?

                /* reject read-only flags */
                if ((ifra->ifra_flags & IN6_IFF_DUPLICATED) != 0 ||
                    (ifra->ifra_flags & IN6_IFF_DETACHED) != 0 ||
+#if !(defined(MOBILE_IPV6) && NMIP > 0)
                    (ifra->ifra_flags & IN6_IFF_NODAD) != 0 ||
+#endif /* MOBILE_IPV6 && NMIP > 0 */
                    (ifra->ifra_flags & IN6_IFF_AUTOCONF) != 0) {
                        return EINVAL;
                }

This deserves the same treatment:

-       if (hostIsNew && in6if_do_dad(ifp))
+       if (hostIsNew && in6if_do_dad(ifp)
+#if defined(MOBILE_IPV6) && NMIP > 0
+           && !(ia->ia6_flags & IN6_IFF_HOME) /* XXX XXX XXX */
+#endif /* MOBILE_IPV6 && NMIP > 0 */
+               )
                ia->ia6_flags |= IN6_IFF_TENTATIVE;

In this case, I suggest extracting static inline subroutine that is empty
#if !defined(MOBILE_IPV6) || NMIP == 0, instead of manually inlining
this code:

+#if defined(MOBILE_IPV6) && NMIP > 0
+       {
+               struct mip6_bul_internal *mbul;
+               while ((mbul = LIST_FIRST(&ia->ia6_mbul_list)) != NULL) {
+                       mip6_bul_remove(mbul);
+               }
+       }
+#endif /* MOBILE_IPV6 && NMIP > 0 */
+

Here, too:

+#if defined(MOBILE_IPV6) && NMIP > 0
+       {
+               /*
+                * XXX skip IPsec policy integrity check if the next
+                * hop is me.  This is dirty but we need this trick
+                * when we use an IPsec tunnel mode policy for
+                * protocol 'any' between a home agent and a mobile
+                * node.
+                */
+               struct in6_ifaddr *ia;
+               
+               for (ia = in6_ifaddr; ia; ia = ia->ia_next) {
+                       if ((ia->ia6_flags & IN6_IFF_NOTREADY) == 0 &&
+                           IN6_ARE_ADDR_EQUAL(&ia->ia_addr.sin6_addr,
+                           &ip6->ip6_dst))
+                               goto skip_ipsec6_in_reject;
+               }
+       }
+#endif /* MOBILE_IPV6 && NMIP > 0 */

Finally, and I realize this is vague, maybe you can get some leverage
out of the linker?

Dave

-- 
David Young             OJC Technologies
dyoung@ojctech.com      Urbana, IL * (217) 278-3933 ext 24