Date: Mon, 06 Dec 1999 21:02:12 +0900 From: Yoshinobu Inoue <shin@nd.net.fujitsu.co.jp> To: asmodai@wxs.nl Cc: freebsd-arch@freebsd.org, cvs-committers@freebsd.org Subject: Re: [Solicite review for KAME 3rd patch] Message-ID: <19991206210212K.shin@nd.net.fujitsu.co.jp> In-Reply-To: <19991204154807.G711@daemon.ninth-circle.org> References: <19991203050711F.shin@nd.net.fujitsu.co.jp> <19991204154807.G711@daemon.ninth-circle.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> Hello Inoue-san,
Hello Mr. Ruigrok,
Thanks for your detailed comments.
> All patches succeeded on a fresh CURRENT source tree except for:
>
> - sbin/ifconfig/Makefile
> - sbin/route/Makefile
> - usr.bin/netstat/Makefile
>
> no .rej file was made curiously enough. Patched by hand.
Was your fix uncommenting
#CFLAGS+=-DINET6
?
Then, I intentionally commented them out by default, because
INET6 kernel config option is also not enabled by default.
> sys/conf/files:
>
> I like the rearranging of the netinet options, it certainly clarifies
> things.
Thanks, though I'm a little bit uncertain where is the best
place ipfilter related files should be put in.
> sys/i386/conf/LINT:
>
> COMPATIBILITY OPTIONS seems to be a whitespace fix. Not needed IMHO.
(and all comments about whitespaces)
I exec'ed some script which do removing whitespaces and tabs
at the end of each line and etc for all files I touched, in
case I mistakenly put them.
But,
> My suggestion: checkout a new source tree and only modify that what you want to
> change and ignore tab line-ups and concentrate on the functionality. White
> space corrections are best done in cosmestic commits.
I agree, I'll take care of whitespaces later.
> Does the gif comment imply that IPv4 in IPv4, IPv4 in IPv6, IPv6 in
> IPv4, IPv6 in IPv6 tunneling is supported? I think it does, but I
> wonder if others might not understand the notation used. One more
> comment line to clarify things from the start wouldn't hurt and would
> save questions to -questions. Also, use `gif' instead of `gif`, minor
> nitpick to remain consistent.
OK, I updated the comment.
> The `faith` pseudo-device catches those packets which sent to it, and
> forwards then to IPv4 and IPv6 translating daemon.
>
> Should this be:
>
> The `faith' pseudo-device captures packets sent to it and forwards them
> to the IPv4/IPv6 translation daemon.
Thanks, I use it.
> Although I wonder if forwards could be changed to divert.
Is the word "divert" more natural?
Then I change the description.
> Something which I wonder about is the removal of opt_inet.h in if.c.
> All other source files seem to retain their opt_inet.h alongside the
> opt_inet6.h. Was this intentional?
It was not included in the first place and add by last KAME
patch, because INET6 was kept in opt_inet.h at that time and
several INET6 related ifdefs was added to if.c.
From this patch, INET6 is kept in opt_inet6.h, so opt_inet.h
inclusion become unnecessary.
> Does faith replace loop? If so, what is against adding the IPv6
> functionality to loop?
Its if_type IFT_FAITH is also important.
Packets catched by faith is put into ip6 input queue and
processed by ip6_input() again.
But this 2nd time input, it matches this check, and treated as
'ours'.
if (ip6_forward_rt.ro_rt && ip6_forward_rt.ro_rt->rt_ifp
&& ip6_forward_rt.ro_rt->rt_ifp->if_type == IFT_FAITH)
ours = 1;
> Also, wouldn't it be better to follow INET6 directly after INET, since
> both are effectively the same protocol, except a different version?
> Just like you did with the #ifdef INET6 later on in this file. Later on
> with the ETHERTYPE cases, you now position yourself between NS and
> DECNET, why not follow directly after INET?
Yes, I fixed it.
> sometimes even revert the previous style(9) compliant code. Removal of
> the $FreeBSD$ doesn't seem needed.
It was my mistake. fixed it.
> And the file suffers from continuous whitespace fixes. Find an update
> of your patch at http://home.wxs.nl/~asmodai/udp-for-ipv6-fixed.19991203 This
> update only fixes the whitespace problems for a couple files. I grew weary
> after file 16 or so.
Woops, sorry and great thanks.
But just one point,
I think this patch should be left, shouldn't it?
-#if defined(INET) || defined(NS) || defined(DECNET) || defined(IPX) || defined(NETATALK)
+#if defined(INET) || defined(INET6) || defined(NS) || defined(DECNET) || defined(IPX) || defined(NETATALK)
> I compiled a world and got:
>
> cc -O -pipe -DKERNEL -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Winline - Wcast-qual -fformat-extensions -ansi -DKLD_MODULE -nostdinc -I- -I/usr/obj/usr/src/sys/modules/if_disc -I/usr/obj/usr/src/sys/modules/if_disc/@ -mpreferred-stack-boundary=2 -c /usr/src/sys/modules/if_disc/../../net/if_disc.c /usr/src/sys/modules/if_disc/../../net/if_disc.c:55: opt_inet6.h: No such file or directory
> *** Error code 1
> Stop in /usr/src/sys/modules/if_disc.
That was also my mistake. I think I fixed it.
> Hope this helps,
Thanks very much again.
When I cvs updated my environments I'll soon prepare new diff.
(But freefall seems to be heavy now.)
Yoshinobu Inoue
> --
> Jeroen Ruigrok van der Werven/Asmodai asmodai(at)wxs.nl
> The BSD Programmer's Documentation Project <http://home.wxs.nl/~asmodai>;
> Network/Security Specialist BSD: Technical excellence at its best
> Learn e-mail netiquette: http://www.lemis.com/email.html
> ...fools rush in where Daemons fear to tread.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?19991206210212K.shin>